Tegra124 CL-DVFS / DFLL clocksource + cpufreq

This patch moves the initialization of PLL_X to be slightly before that of CCLK_G. This ensures that at boot, CCLK_G will immediately have a parent and the common clock framework can determine its clock rate correctly.

Without this patch, calling clk_put on CCLK_G could cause the CCF to set its rate to zero, hanging the system.

Signed-off-by Mikko Perttunen <[email protected]>

---
v8:
- Added

 drivers/clk/tegra/clk-tegra-super-gen4.c | 46 ++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 20 deletions(-)

drivers/clk/tegra/clk-tegra-super-gen4.c +26 -20

index f1f4410..c5ea9ee 100644
@@ -104,6 +104,32 @@ void __init tegra_super_clk_gen4_init(void __iomem *clk_base,
        struct clk *clk;
        struct clk **dt_clk;
 
+       /*
+        * Register PLL_X first so that CCLK_G has a parent at registration
+        * time. This ensures that the common clock framework knows CCLK_G's
+        * rate.
+        */
+
+#if defined(CONFIG_ARCH_TEGRA_114_SOC) || defined(CONFIG_ARCH_TEGRA_124_SOC)
+       /* PLLX */
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
+       if (!dt_clk)
+               return;
+
+       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
+                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
+       *dt_clk = clk;
+
+       /* PLLX_OUT0 */
+
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
+       if (!dt_clk)
+               return;
+       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
+                                       CLK_SET_RATE_PARENT, 1, 2);
+       *dt_clk = clk;
+#endif
+
        /* CCLKG */
        dt_clk = tegra_lookup_dt_id(tegra_clk_cclk_g, tegra_clks);
        if (dt_clk) {

@@ -127,25 +153,5 @@ void __init tegra_super_clk_gen4_init(void __iomem *clk_base,
        }
 
        tegra_sclk_init(clk_base, tegra_clks);
-
-#if defined(CONFIG_ARCH_TEGRA_114_SOC) || defined(CONFIG_ARCH_TEGRA_124_SOC)
-       /* PLLX */
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
-       if (!dt_clk)
-               return;
-
-       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
-                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
-       *dt_clk = clk;
-
-       /* PLLX_OUT0 */
-
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
-       if (!dt_clk)
-               return;
-       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
-                                       CLK_SET_RATE_PARENT, 1, 2);
-       *dt_clk = clk;
-#endif
 }
 

Signature · 1 line

2.3.0

Quoting Mikko Perttunen (2015-03-01 04:44:33)

Quoted 7 lines

This patch moves the initialization of PLL_X to be slightly before that of CCLK_G. This ensures that at boot, CCLK_G will immediately have a parent and the common clock framework can determine its clock rate correctly.

Without this patch, calling clk_put on CCLK_G could cause the CCF to set its rate to zero, hanging the system.

Hi Mikko,

Patch looks fine to me but I wanted to get more info on the behavior you mentioned above about clk_put. Is there some special circumstance that causes this for you? Why does calling clk_put adjust the rate of your clock?

Thanks,
Mike

Quoted 76 lines

Signed-off-by Mikko Perttunen <[email protected]>

---
v8:
- Added

  drivers/clk/tegra/clk-tegra-super-gen4.c | 46 ++++++++++++++++++--------------
  1 file changed, 26 insertions(+), 20 deletions(-)

drivers/clk/tegra/clk-tegra-super-gen4.c +26 -21

index f1f4410..c5ea9ee 100644
@@ -104,6 +104,32 @@ void __init tegra_super_clk_gen4_init(void __iomem *clk_base,
         struct clk *clk;
         struct clk **dt_clk;
 
+       /*
+        * Register PLL_X first so that CCLK_G has a parent at registration
+        * time. This ensures that the common clock framework knows CCLK_G's
+        * rate.
+        */
+
+#if defined(CONFIG_ARCH_TEGRA_114_SOC) || defined(CONFIG_ARCH_TEGRA_124_SOC)
+       /* PLLX */
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
+       if (!dt_clk)
+               return;
+
+       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
+                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
+       *dt_clk = clk;
+
+       /* PLLX_OUT0 */
+
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
+       if (!dt_clk)
+               return;
+       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
+                                       CLK_SET_RATE_PARENT, 1, 2);
+       *dt_clk = clk;
+#endif
+
         /* CCLKG */
         dt_clk = tegra_lookup_dt_id(tegra_clk_cclk_g, tegra_clks);
         if (dt_clk) {

@@ -127,25 +153,5 @@ void __init tegra_super_clk_gen4_init(void __iomem *clk_base,
         }
 
         tegra_sclk_init(clk_base, tegra_clks);
-
-#if defined(CONFIG_ARCH_TEGRA_114_SOC) || defined(CONFIG_ARCH_TEGRA_124_SOC)
-       /* PLLX */
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
-       if (!dt_clk)
-               return;
-
-       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
-                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
-       *dt_clk = clk;
-
-       /* PLLX_OUT0 */
-
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
-       if (!dt_clk)
-               return;
-       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
-                                       CLK_SET_RATE_PARENT, 1, 2);
-       *dt_clk = clk;
-#endif
  }
 
--

2.3.0

Quoted 14 lines · Michael Turquette

Quoting Mikko Perttunen (2015-03-01 04:44:33)

Quoted 8 lines

This patch moves the initialization of PLL_X to be slightly before that of CCLK_G. This ensures that at boot, CCLK_G will immediately have a parent and the common clock framework can determine its clock rate correctly.

Without this patch, calling clk_put on CCLK_G could cause the CCF to set its rate to zero, hanging the system.

Hi Mikko,

Quoted 7 lines

Patch looks fine to me but I wanted to get more info on the behavior you mentioned above about clk_put. Is there some special circumstance that causes this for you? Why does calling clk_put adjust the rate of your clock?

Thanks,
Mike

Hi Mike,

this is the chain of events:
- CCLK_G is registered. CCF stores its current rate, but since it doesn't have a parent at this point, the rate is assumed zero.
- tegra cpufreq driver tries to probe, and clk_gets CCLK_G
- tegra dfll driver tries to probe, but fails
- tegra cpufreq driver's probe fails, and during unwinding clk_puts CCLK_G
- CCF attempts to restore CCLK_G's rate to what it was prior to the clk_get (to revert possible changes due to clock constraints)

- the stored rate was zero, so CCLK_G is set to zero.

We did discuss it a bit on IRC with Tomeu and Peter and agreed that some fix in CCF should be done, but we didn't get much further than that.

Mikko

Quoted 81 lines

Quoted 81 lines

Signed-off-by Mikko Perttunen <[email protected]>

---
v8:
- Added

  drivers/clk/tegra/clk-tegra-super-gen4.c | 46
++++++++++++++++++--------------
  1 file changed, 26 insertions(+), 20 deletions(-)

drivers/clk/tegra/clk-tegra-super-gen4.c +2 -0

index f1f4410..c5ea9ee 100644
@@ -104,6 +104,32 @@ void __init tegra_super_clk_gen4_init(void __iomem
 *clk_base,
         struct clk *clk;
         struct clk **dt_clk;
 
+       /*
+        * Register PLL_X first so that CCLK_G has a parent at

registration
+ * time. This ensures that the common clock framework knows
CCLK_G's

drivers/clk/tegra/clk-tegra-super-gen4.c +4 -0

+        * rate.
+        */
+
+#if defined(CONFIG_ARCH_TEGRA_114_SOC) ||

defined(CONFIG_ARCH_TEGRA_124_SOC)

drivers/clk/tegra/clk-tegra-super-gen4.c +19 -20

+       /* PLLX */
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
+       if (!dt_clk)
+               return;
+
+       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
+                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
+       *dt_clk = clk;
+
+       /* PLLX_OUT0 */
+
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
+       if (!dt_clk)
+               return;
+       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
+                                       CLK_SET_RATE_PARENT, 1, 2);
+       *dt_clk = clk;
+#endif
+
         /* CCLKG */
         dt_clk = tegra_lookup_dt_id(tegra_clk_cclk_g, tegra_clks);
         if (dt_clk) {

@@ -127,25 +153,5 @@ void __init tegra_super_clk_gen4_init(void __iomem
 *clk_base,
         }
 
         tegra_sclk_init(clk_base, tegra_clks);
-
-#if defined(CONFIG_ARCH_TEGRA_114_SOC) ||
 defined(CONFIG_ARCH_TEGRA_124_SOC)
-       /* PLLX */
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
-       if (!dt_clk)
-               return;
-
-       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
-                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
-       *dt_clk = clk;
-
-       /* PLLX_OUT0 */
-
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
-       if (!dt_clk)
-               return;
-       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
-                                       CLK_SET_RATE_PARENT, 1, 2);
-       *dt_clk = clk;
-#endif

  }

Quoted 3 lines

Quoted 3 lines

--
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 26 lines · Michael Turquette

Quoted 14 lines

Quoting Mikko Perttunen (2015-03-01 04:44:33)

Quoted 8 lines

This patch moves the initialization of PLL_X to be slightly before that of CCLK_G. This ensures that at boot, CCLK_G will immediately have a parent and the common clock framework can determine its clock rate correctly.

Without this patch, calling clk_put on CCLK_G could cause the CCF to set its rate to zero, hanging the system.

Hi Mikko,

Quoted 7 lines

Patch looks fine to me but I wanted to get more info on the behavior you mentioned above about clk_put. Is there some special circumstance that causes this for you? Why does calling clk_put adjust the rate of your clock?

Thanks,
Mike

Hi Mike,

Quoted 8 lines

this is the chain of events:
- CCLK_G is registered. CCF stores its current rate, but since it doesn't have a parent at this point, the rate is assumed zero.
- tegra cpufreq driver tries to probe, and clk_gets CCLK_G
- tegra dfll driver tries to probe, but fails
- tegra cpufreq driver's probe fails, and during unwinding clk_puts CCLK_G
- CCF attempts to restore CCLK_G's rate to what it was prior to the clk_get (to revert possible changes due to clock constraints)

The CCF will currently only do so if any constraints were set in the per-user clk that was destroyed, so this particular problem shouldn't happen any more: ec02ace clk: Only recalculate the rate if needed

Quoted 5 lines

- the stored rate was zero, so CCLK_G is set to zero.

We did discuss it a bit on IRC with Tomeu and Peter and agreed that some fix in CCF should be done, but we didn't get much further than that.

Wonder if we could somehow make sure that the rate in the CCF matches the current state of the HW.

Regards,

Tomeu

Quoted 93 lines

Mikko

Quoted 81 lines

Quoted 81 lines

Signed-off-by Mikko Perttunen <[email protected]>

---
v8:
- Added

  drivers/clk/tegra/clk-tegra-super-gen4.c | 46
++++++++++++++++++--------------
  1 file changed, 26 insertions(+), 20 deletions(-)

drivers/clk/tegra/clk-tegra-super-gen4.c +2 -0

index f1f4410..c5ea9ee 100644
@@ -104,6 +104,32 @@ void __init tegra_super_clk_gen4_init(void __iomem
 *clk_base,
         struct clk *clk;
         struct clk **dt_clk;
 
+       /*
+        * Register PLL_X first so that CCLK_G has a parent at

registration
+ * time. This ensures that the common clock framework knows
CCLK_G's

drivers/clk/tegra/clk-tegra-super-gen4.c +4 -0

+        * rate.
+        */
+
+#if defined(CONFIG_ARCH_TEGRA_114_SOC) ||

defined(CONFIG_ARCH_TEGRA_124_SOC)

drivers/clk/tegra/clk-tegra-super-gen4.c +19 -20

+       /* PLLX */
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
+       if (!dt_clk)
+               return;
+
+       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
+                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
+       *dt_clk = clk;
+
+       /* PLLX_OUT0 */
+
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
+       if (!dt_clk)
+               return;
+       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
+                                       CLK_SET_RATE_PARENT, 1, 2);
+       *dt_clk = clk;
+#endif
+
         /* CCLKG */
         dt_clk = tegra_lookup_dt_id(tegra_clk_cclk_g, tegra_clks);
         if (dt_clk) {

@@ -127,25 +153,5 @@ void __init tegra_super_clk_gen4_init(void __iomem
 *clk_base,
         }
 
         tegra_sclk_init(clk_base, tegra_clks);
-
-#if defined(CONFIG_ARCH_TEGRA_114_SOC) ||
 defined(CONFIG_ARCH_TEGRA_124_SOC)
-       /* PLLX */
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
-       if (!dt_clk)
-               return;
-
-       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
-                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
-       *dt_clk = clk;
-
-       /* PLLX_OUT0 */
-
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
-       if (!dt_clk)
-               return;
-       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
-                                       CLK_SET_RATE_PARENT, 1, 2);
-       *dt_clk = clk;
-#endif

  }

Quoted 3 lines

Quoted 3 lines

--
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoting Tomeu Vizoso (2015-04-13 05:17:01)

Quoted 36 lines · Mikko Perttunen

Quoted 34 lines · Michael Turquette

Quoted 14 lines

Quoting Mikko Perttunen (2015-03-01 04:44:33)

Quoted 8 lines

This patch moves the initialization of PLL_X to be slightly before that of CCLK_G. This ensures that at boot, CCLK_G will immediately have a parent and the common clock framework can determine its clock rate correctly.

Without this patch, calling clk_put on CCLK_G could cause the CCF to set its rate to zero, hanging the system.

Hi Mikko,

Quoted 7 lines

Patch looks fine to me but I wanted to get more info on the behavior you mentioned above about clk_put. Is there some special circumstance that causes this for you? Why does calling clk_put adjust the rate of your clock?

Thanks,
Mike

Hi Mike,

this is the chain of events:
- CCLK_G is registered. CCF stores its current rate, but since it doesn't have a parent at this point, the rate is assumed zero.
- tegra cpufreq driver tries to probe, and clk_gets CCLK_G
- tegra dfll driver tries to probe, but fails
- tegra cpufreq driver's probe fails, and during unwinding clk_puts CCLK_G
- CCF attempts to restore CCLK_G's rate to what it was prior to the clk_get (to revert possible changes due to clock constraints)

Quoted 3 lines

The CCF will currently only do so if any constraints were set in the per-user clk that was destroyed, so this particular problem shouldn't happen any more: ec02ace clk: Only recalculate the rate if needed

Precisely. clk_put shouldn't be setting rates unless we're releasing a constraint. Can this re-ordering patch be dropped?

Quoted 9 lines

Quoted 6 lines

- the stored rate was zero, so CCLK_G is set to zero.

We did discuss it a bit on IRC with Tomeu and Peter and agreed that some fix in CCF should be done, but we didn't get much further than that.

Wonder if we could somehow make sure that the rate in the CCF matches the current state of the HW.

If there is no constraint expressed by Linux software then we should not care.

Probably we need to track a few more things in the framework. Stephen and I were discussing struct clk.hw_en which tracks the enable/disable state of the hardware independently of the enable_count (e.g. gate clock is enabled by bootloader but not enabled by Linux ccf).

Tracking something like "is_clk_initialized" would be helpful. It is an abstract concept but knowing that clock has been successfully hooked up to its parent and that its rate has been properly calculated would be very helpful for some corner cases.

Regards,
Mike

Quoted 98 lines

Regards,

Tomeu

Quoted 93 lines

Mikko

Quoted 81 lines

Quoted 81 lines

Signed-off-by Mikko Perttunen <[email protected]>

---
v8:
- Added

   drivers/clk/tegra/clk-tegra-super-gen4.c | 46
++++++++++++++++++--------------
   1 file changed, 26 insertions(+), 20 deletions(-)

drivers/clk/tegra/clk-tegra-super-gen4.c +2 -0

index f1f4410..c5ea9ee 100644
@@ -104,6 +104,32 @@ void __init tegra_super_clk_gen4_init(void __iomem
 *clk_base,
          struct clk *clk;
          struct clk **dt_clk;
 
+       /*
+        * Register PLL_X first so that CCLK_G has a parent at

registration
+ * time. This ensures that the common clock framework knows
CCLK_G's

drivers/clk/tegra/clk-tegra-super-gen4.c +4 -0

+        * rate.
+        */
+
+#if defined(CONFIG_ARCH_TEGRA_114_SOC) ||

defined(CONFIG_ARCH_TEGRA_124_SOC)

drivers/clk/tegra/clk-tegra-super-gen4.c +19 -20

+       /* PLLX */
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
+       if (!dt_clk)
+               return;
+
+       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
+                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
+       *dt_clk = clk;
+
+       /* PLLX_OUT0 */
+
+       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
+       if (!dt_clk)
+               return;
+       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
+                                       CLK_SET_RATE_PARENT, 1, 2);
+       *dt_clk = clk;
+#endif
+
          /* CCLKG */
          dt_clk = tegra_lookup_dt_id(tegra_clk_cclk_g, tegra_clks);
          if (dt_clk) {

@@ -127,25 +153,5 @@ void __init tegra_super_clk_gen4_init(void __iomem
 *clk_base,
          }
 
          tegra_sclk_init(clk_base, tegra_clks);
-
-#if defined(CONFIG_ARCH_TEGRA_114_SOC) ||
 defined(CONFIG_ARCH_TEGRA_124_SOC)
-       /* PLLX */
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x, tegra_clks);
-       if (!dt_clk)
-               return;
-
-       clk = tegra_clk_register_pllxc("pll_x", "pll_ref", clk_base,
-                       pmc_base, CLK_IGNORE_UNUSED, params, NULL);
-       *dt_clk = clk;
-
-       /* PLLX_OUT0 */
-
-       dt_clk = tegra_lookup_dt_id(tegra_clk_pll_x_out0, tegra_clks);
-       if (!dt_clk)
-               return;
-       clk = clk_register_fixed_factor(NULL, "pll_x_out0", "pll_x",
-                                       CLK_SET_RATE_PARENT, 1, 2);
-       *dt_clk = clk;
-#endif

   }

Quoted 3 lines

Quoted 3 lines

--
2.3.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

drivers/cpufreq/Kconfig.arm +5 -0

+	depends on ARCH_TEGRA && CPUFREQ_DT
+	default y
+	help
+	  This adds the CPUFreq driver support for Tegra124 SOCs.
+
 config ARM_PXA2xx_CPUFREQ
 	tristate "Intel PXA2xx CPUfreq driver"
 	depends on PXA27x || PXA25x

drivers/cpufreq/Makefile +0 -0

index 312d9c3..d478b83 100644

drivers/cpufreq/Makefile +0 -0

 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
 
 ##################################################################################

drivers/cpufreq/tegra124-cpufreq.c new file +11 -0

new file mode 100644
index 0000000..45778ce
@@ -0,0 +1,217 @@
+/*
+ * Tegra 124 cpufreq driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.

drivers/cpufreq/tegra124-cpufreq.c new file +6 -0

+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>

drivers/cpufreq/tegra124-cpufreq.c new file +4 -0

+	},
+	.probe		= tegra124_cpufreq_probe,
+	.remove		= tegra124_cpufreq_remove,
+};

+module_init(tegra_cpufreq_init);

drivers/cpufreq/Kconfig.arm config ARM_TEGRA20_CPUFREQ · line 260

Quoted 8 lines · Mikko Perttunen

drivers/cpufreq/Kconfig.arm +2 -0

@@ -256,6 +256,13 @@ config ARM_TEGRA20_CPUFREQ
  	help
  	  This adds the CPUFreq driver support for Tegra20 SOCs.
 
+config ARM_TEGRA124_CPUFREQ
+	bool "Tegra124 CPUFreq support"

This adds a bool Kconfig symbol...

Quoted 13 lines

drivers/cpufreq/Kconfig.arm +5 -0

+	depends on ARCH_TEGRA && CPUFREQ_DT
+	default y
+	help
+	  This adds the CPUFreq driver support for Tegra124 SOCs.
+
  config ARM_PXA2xx_CPUFREQ
  	tristate "Intel PXA2xx CPUfreq driver"
  	depends on PXA27x || PXA25x

drivers/cpufreq/Makefile +0 -0

index 312d9c3..d478b83 100644

drivers/cpufreq/Makefile obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o · line 80

Quoted 4 lines

drivers/cpufreq/Makefile +1 -0

@@ -77,6 +77,7 @@ 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_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
+obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o

... so this object will either not be built or be built-in.

Quoted 20 lines

drivers/cpufreq/Makefile +0 -0

  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
 
  ##################################################################################

drivers/cpufreq/tegra124-cpufreq.c new file +11 -0

new file mode 100644
index 0000000..45778ce
@@ -0,0 +1,217 @@
+/*
+ * Tegra 124 cpufreq driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.

drivers/cpufreq/tegra124-cpufreq.c line 21

Quoted 10 lines

drivers/cpufreq/tegra124-cpufreq.c new file +10 -0

+ */
+
+#define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
+
+#include <linux/clk.h>
+#include <linux/cpufreq-dt.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>

Is linux/module.h needed?

Quoted 6 lines

drivers/cpufreq/tegra124-cpufreq.c new file +6 -0

+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>

[...]

drivers/cpufreq/tegra124-cpufreq.c line 183

Quoted 4 lines

drivers/cpufreq/tegra124-cpufreq.c new file +4 -0

+static struct platform_driver tegra124_cpufreq_platdrv = {
+	.driver = {
+		.name	= "cpufreq-tegra124",
+		.owner	= THIS_MODULE,

.owner will always be, in short, NULL.

Quoted 4 lines

drivers/cpufreq/tegra124-cpufreq.c new file +4 -0

+	},
+	.probe		= tegra124_cpufreq_probe,
+	.remove		= tegra124_cpufreq_remove,
+};

[...]

Quoted 1 line

+module_init(tegra_cpufreq_init);

This might as well be, I think, device_initcall().

drivers/cpufreq/tegra124-cpufreq.c line 217

Quoted 3 lines

drivers/cpufreq/tegra124-cpufreq.c new file +3 -0

+MODULE_AUTHOR("Tuomas Tynkkynen <[email protected]>");
+MODULE_DESCRIPTION("cpufreq driver for NVIDIA Tegra124");
+MODULE_LICENSE("GPL v2");

The strings in these macros will always be preprocessed away, because this driver cannot become a module.

Paul Bolle

Quoted 4 lines · Paul Bolle

On Sun, 2015-03-01 at 14:44 +0200, Mikko Perttunen wrote:

[...]

Quoted 7 lines

drivers/cpufreq/tegra124-cpufreq.c line 183

Quoted 4 lines

drivers/cpufreq/tegra124-cpufreq.c new file +4 -0

+static struct platform_driver tegra124_cpufreq_platdrv = {
+    .driver = {
+        .name    = "cpufreq-tegra124",
+        .owner    = THIS_MODULE,

.owner will always be, in short, NULL.

Well noticed! I changed the config symbol to be tristate.

Thanks,
Mikko Perttunen

Quoted 10 lines · Mikko Perttunen

Quoted 9 lines · Paul Bolle

On Sun, 2015-03-01 at 14:44 +0200, Mikko Perttunen wrote:

[...]

Quoted 4 lines

drivers/cpufreq/tegra124-cpufreq.c new file +4 -0

+static struct platform_driver tegra124_cpufreq_platdrv = {
+    .driver = {
+        .name    = "cpufreq-tegra124",
+        .owner    = THIS_MODULE,

Quoted 2 lines

Quoted 2 lines

.owner will always be, in short, NULL.

Which reminds me: recently there have been cleanups that remove the .owner field assignments (e.g. 30f51f248c1d "cpufreq: drop owner assignment from platform_drivers"), so it can be removed from here as well.

Quoted 6 lines

Well noticed! I changed the config symbol to be tristate.

Thanks,
Mikko Perttunen

Hi, this is v8 of the Tegra124 cpufreq series. Aside rebasing on latest -next, the following changes have been done:

clk: tegra: Add binding for the Tegra124 DFLL clocksource
- Changed dfll@ -> clock@
- Changed compatibility string to "nvidia,tegra124-dfll"
- Clarified how the vdd-cpu-supply property is used
- Marked nvidia,cg-scale as optional since it is a boolean property
- Expanded the 'FS mode' term to 'full speed mode'
- Added dvco reset control

ARM: tegra: Add the DFLL to Tegra124 device tree
- Changed dfll@ -> clock@
- Added dvco reset control

ARM: tegra: Enable the DFLL on the Jetson TK1
- Changed dfll@ -> clock@

clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent
- Added

clk: tegra: Introduce ability for SoC-specific reset control callbacks
- Added

clk: tegra: Add DFLL DVCO reset control for Tegra124
- Changed to use SoC-specific reset control callback

clk: tegra: Add Tegra124 DFLL clocksource platform driver
- Don't set DVCO reset handlers

clk: tegra: Add library for the DFLL clock source (open-loop mode)
- Use reset control instead of function pointers

also added acks from v7.

The series is available in a git repository at
git://github.com/cyndis/linux.git cldvfs-v8

Tested by me on Jetson-TK1 (rev. D).

Original cover letter:

This series implements the DFLL/CL-DVFS clock source for the fast CPU cluster on Tegra124, and a cpufreq driver that uses the DFLL for clocking the CPU. Most of this is based on Paul Walmsley's public patch set from December 2013, which is available at
http://comments.gmane.org/gmane.linux.ports.tegra/15273

The DFLL clock hardware is a voltage-controlled oscillator plus control logic that compares the generated output clock with a 51 MHz reference clock, and can make decisions to either lower or raise the DFLL voltage to keep the output rate close to the software-requested rate. The voltage changes are done by communicating with an off-chip PMIC via either I2C or PWM. As the DFLL oscillator is powered via the CPU rail, using the DFLL as the CPU clocksource also gives us dynamic CPU voltage scaling.

This series has been tested on the Jetson TK1 (Rev C). Porting this to the Venice2 should be simple, though do note that it does not have active cooling.

Thanks,
Tuomas

Mikko Perttunen (3):
clk: tegra: Introduce ability for SoC-specific reset control callbacks
clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent
ARM: tegra: Add CPU regulator to the Jetson TK1 device tree

Paul Walmsley (1):
clk: tegra: Add DFLL DVCO reset control for Tegra124

Tuomas Tynkkynen (14):
clk: tegra: Add binding for the Tegra124 DFLL clocksource
clk: tegra: Add library for the DFLL clock source (open-loop mode)
clk: tegra: Add closed loop support for the DFLL
clk: tegra: Add functions for parsing CVB tables
clk: tegra: Add Tegra124 DFLL clocksource platform driver
clk: tegra: Save/restore CCLKG_BURST_POLICY on suspend
clk: tegra: Add the DFLL as a possible parent of the cclk_g clock
ARM: tegra: Add the DFLL to Tegra124 device tree
ARM: tegra: Enable the DFLL on the Jetson TK1
cpufreq: tegra124: Add device tree bindings
cpufreq: tegra: Rename tegra-cpufreq to tegra20-cpufreq
cpufreq: Add cpufreq driver for Tegra124
ARM: tegra: Add entries for cpufreq on Tegra124
ARM: tegra: enable Tegra124 cpufreq driver by default

 .../bindings/clock/nvidia,tegra124-dfll.txt        |   79 +
 .../bindings/cpufreq/tegra124-cpufreq.txt          |   44 +
 arch/arm/boot/dts/tegra124-jetson-tk1.dts          |   15 +-
 arch/arm/boot/dts/tegra124.dtsi                    |   34 +
 arch/arm/configs/tegra_defconfig                   |    1 +
 arch/arm/mach-tegra/Kconfig                        |    1 +
 drivers/clk/tegra/Makefile                         |    3 +
 drivers/clk/tegra/clk-dfll.c                       | 1755 ++++++++++++++++++++
 drivers/clk/tegra/clk-dfll.h                       |   54 +
 drivers/clk/tegra/clk-tegra-super-gen4.c           |   50 +-
 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c         |  163 ++
 drivers/clk/tegra/clk-tegra124.c                   |   82 +
 drivers/clk/tegra/clk.c                            |   36 +-
 drivers/clk/tegra/clk.h                            |    3 +
 drivers/clk/tegra/cvb.c                            |  133 ++
 drivers/clk/tegra/cvb.h                            |   67 +
 drivers/cpufreq/Kconfig.arm                        |   13 +-
 drivers/cpufreq/Makefile                           |    3 +-
 drivers/cpufreq/tegra124-cpufreq.c                 |  217 +++
 .../cpufreq/{tegra-cpufreq.c => tegra20-cpufreq.c} |    0
 include/dt-bindings/reset/tegra124-car.h           |   11 +
 21 files changed, 2730 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
 create mode 100644 Documentation/devicetree/bindings/cpufreq/tegra124-cpufreq.txt
 create mode 100644 drivers/clk/tegra/clk-dfll.c
 create mode 100644 drivers/clk/tegra/clk-dfll.h
 create mode 100644 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
 create mode 100644 drivers/clk/tegra/cvb.c
 create mode 100644 drivers/clk/tegra/cvb.h
 create mode 100644 drivers/cpufreq/tegra124-cpufreq.c
 rename drivers/cpufreq/{tegra-cpufreq.c => tegra20-cpufreq.c} (100%)
 create mode 100644 include/dt-bindings/reset/tegra124-car.h

--
2.3.0

Hi Mike,

Have you had a chance to look at these changes to the Tegra clock driver? If you're fine with it, I'd like to take these patches through the Tegra tree because the rest of the series depends on them. I can provide a stable branch in case we need to base other Tegra clock changes on top of this.

Thierry

Quoted 125 lines · Mikko Perttunen

Hi, this is v8 of the Tegra124 cpufreq series. Aside rebasing on latest -next, the following changes have been done:

clk: tegra: Add binding for the Tegra124 DFLL clocksource
- Changed dfll@ -> clock@
- Changed compatibility string to "nvidia,tegra124-dfll"
- Clarified how the vdd-cpu-supply property is used
- Marked nvidia,cg-scale as optional since it is a boolean property
- Expanded the 'FS mode' term to 'full speed mode'
- Added dvco reset control

ARM: tegra: Add the DFLL to Tegra124 device tree
- Changed dfll@ -> clock@
- Added dvco reset control

ARM: tegra: Enable the DFLL on the Jetson TK1
- Changed dfll@ -> clock@

clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent
- Added

clk: tegra: Introduce ability for SoC-specific reset control callbacks
- Added

clk: tegra: Add DFLL DVCO reset control for Tegra124
- Changed to use SoC-specific reset control callback

clk: tegra: Add Tegra124 DFLL clocksource platform driver
- Don't set DVCO reset handlers

clk: tegra: Add library for the DFLL clock source (open-loop mode)
- Use reset control instead of function pointers

also added acks from v7.

The series is available in a git repository at
git://github.com/cyndis/linux.git cldvfs-v8

Tested by me on Jetson-TK1 (rev. D).

Original cover letter:

This series implements the DFLL/CL-DVFS clock source for the fast CPU cluster on Tegra124, and a cpufreq driver that uses the DFLL for clocking the CPU. Most of this is based on Paul Walmsley's public patch set from December 2013, which is available at
http://comments.gmane.org/gmane.linux.ports.tegra/15273

The DFLL clock hardware is a voltage-controlled oscillator plus control logic that compares the generated output clock with a 51 MHz reference clock, and can make decisions to either lower or raise the DFLL voltage to keep the output rate close to the software-requested rate. The voltage changes are done by communicating with an off-chip PMIC via either I2C or PWM. As the DFLL oscillator is powered via the CPU rail, using the DFLL as the CPU clocksource also gives us dynamic CPU voltage scaling.

This series has been tested on the Jetson TK1 (Rev C). Porting this to the Venice2 should be simple, though do note that it does not have active cooling.

Thanks,
Tuomas

Mikko Perttunen (3):
clk: tegra: Introduce ability for SoC-specific reset control callbacks
clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent
ARM: tegra: Add CPU regulator to the Jetson TK1 device tree

Paul Walmsley (1):
clk: tegra: Add DFLL DVCO reset control for Tegra124

Tuomas Tynkkynen (14):
clk: tegra: Add binding for the Tegra124 DFLL clocksource
clk: tegra: Add library for the DFLL clock source (open-loop mode)
clk: tegra: Add closed loop support for the DFLL
clk: tegra: Add functions for parsing CVB tables
clk: tegra: Add Tegra124 DFLL clocksource platform driver
clk: tegra: Save/restore CCLKG_BURST_POLICY on suspend
clk: tegra: Add the DFLL as a possible parent of the cclk_g clock
ARM: tegra: Add the DFLL to Tegra124 device tree
ARM: tegra: Enable the DFLL on the Jetson TK1
cpufreq: tegra124: Add device tree bindings
cpufreq: tegra: Rename tegra-cpufreq to tegra20-cpufreq
cpufreq: Add cpufreq driver for Tegra124
ARM: tegra: Add entries for cpufreq on Tegra124
ARM: tegra: enable Tegra124 cpufreq driver by default

 .../bindings/clock/nvidia,tegra124-dfll.txt        |   79 +
 .../bindings/cpufreq/tegra124-cpufreq.txt          |   44 +
 arch/arm/boot/dts/tegra124-jetson-tk1.dts          |   15 +-
 arch/arm/boot/dts/tegra124.dtsi                    |   34 +
 arch/arm/configs/tegra_defconfig                   |    1 +
 arch/arm/mach-tegra/Kconfig                        |    1 +
 drivers/clk/tegra/Makefile                         |    3 +
 drivers/clk/tegra/clk-dfll.c                       | 1755 ++++++++++++++++++++
 drivers/clk/tegra/clk-dfll.h                       |   54 +
 drivers/clk/tegra/clk-tegra-super-gen4.c           |   50 +-
 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c         |  163 ++
 drivers/clk/tegra/clk-tegra124.c                   |   82 +
 drivers/clk/tegra/clk.c                            |   36 +-
 drivers/clk/tegra/clk.h                            |    3 +
 drivers/clk/tegra/cvb.c                            |  133 ++
 drivers/clk/tegra/cvb.h                            |   67 +
 drivers/cpufreq/Kconfig.arm                        |   13 +-
 drivers/cpufreq/Makefile                           |    3 +-
 drivers/cpufreq/tegra124-cpufreq.c                 |  217 +++
 .../cpufreq/{tegra-cpufreq.c => tegra20-cpufreq.c} |    0
 include/dt-bindings/reset/tegra124-car.h           |   11 +
 21 files changed, 2730 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
 create mode 100644 Documentation/devicetree/bindings/cpufreq/tegra124-cpufreq.txt
 create mode 100644 drivers/clk/tegra/clk-dfll.c
 create mode 100644 drivers/clk/tegra/clk-dfll.h
 create mode 100644 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
 create mode 100644 drivers/clk/tegra/cvb.c
 create mode 100644 drivers/clk/tegra/cvb.h
 create mode 100644 drivers/cpufreq/tegra124-cpufreq.c
 rename drivers/cpufreq/{tegra-cpufreq.c => tegra20-cpufreq.c} (100%)
 create mode 100644 include/dt-bindings/reset/tegra124-car.h

--
2.3.0

Quoting Thierry Reding (2015-03-11 03:07:43)

Quoted 7 lines

Hi Mike,

Have you had a chance to look at these changes to the Tegra clock driver? If you're fine with it, I'd like to take these patches through the Tegra tree because the rest of the series depends on them. I can provide a stable branch in case we need to base other Tegra clock changes on top of this.

Hi Thierry,

Clock patches (and corresponding DT binding descriptions and changes to DTS) look fine to me. Please add:

Acked-by Michael Turquette <[email protected]>

I did have a question about the beahvior of clk_put in one of Mikko's patches but it should not gate this series. I'm just trying to find out if we have a bug in the framework or if the Tegra driver is a special case.

Also I do not think a stable branch is necessary.

Regards,
Mike

Quoted 129 lines

Thierry

Quoted 125 lines · Mikko Perttunen

Hi, this is v8 of the Tegra124 cpufreq series. Aside rebasing on latest -next, the following changes have been done:

clk: tegra: Add binding for the Tegra124 DFLL clocksource
- Changed dfll@ -> clock@
- Changed compatibility string to "nvidia,tegra124-dfll"
- Clarified how the vdd-cpu-supply property is used
- Marked nvidia,cg-scale as optional since it is a boolean property
- Expanded the 'FS mode' term to 'full speed mode'
- Added dvco reset control

ARM: tegra: Add the DFLL to Tegra124 device tree
- Changed dfll@ -> clock@
- Added dvco reset control

ARM: tegra: Enable the DFLL on the Jetson TK1
- Changed dfll@ -> clock@

clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent
- Added

clk: tegra: Introduce ability for SoC-specific reset control callbacks
- Added

clk: tegra: Add DFLL DVCO reset control for Tegra124
- Changed to use SoC-specific reset control callback

clk: tegra: Add Tegra124 DFLL clocksource platform driver
- Don't set DVCO reset handlers

clk: tegra: Add library for the DFLL clock source (open-loop mode)
- Use reset control instead of function pointers

also added acks from v7.

The series is available in a git repository at
git://github.com/cyndis/linux.git cldvfs-v8

Tested by me on Jetson-TK1 (rev. D).

Original cover letter:

This series implements the DFLL/CL-DVFS clock source for the fast CPU cluster on Tegra124, and a cpufreq driver that uses the DFLL for clocking the CPU. Most of this is based on Paul Walmsley's public patch set from December 2013, which is available at
http://comments.gmane.org/gmane.linux.ports.tegra/15273

The DFLL clock hardware is a voltage-controlled oscillator plus control logic that compares the generated output clock with a 51 MHz reference clock, and can make decisions to either lower or raise the DFLL voltage to keep the output rate close to the software-requested rate. The voltage changes are done by communicating with an off-chip PMIC via either I2C or PWM. As the DFLL oscillator is powered via the CPU rail, using the DFLL as the CPU clocksource also gives us dynamic CPU voltage scaling.

This series has been tested on the Jetson TK1 (Rev C). Porting this to the Venice2 should be simple, though do note that it does not have active cooling.

Thanks,
Tuomas

Mikko Perttunen (3):
clk: tegra: Introduce ability for SoC-specific reset control callbacks
clk: tegra: Initialize PLL_X before CCLK_G to ensure it has a parent
ARM: tegra: Add CPU regulator to the Jetson TK1 device tree

Paul Walmsley (1):
clk: tegra: Add DFLL DVCO reset control for Tegra124

Tuomas Tynkkynen (14):
clk: tegra: Add binding for the Tegra124 DFLL clocksource
clk: tegra: Add library for the DFLL clock source (open-loop mode)
clk: tegra: Add closed loop support for the DFLL
clk: tegra: Add functions for parsing CVB tables
clk: tegra: Add Tegra124 DFLL clocksource platform driver
clk: tegra: Save/restore CCLKG_BURST_POLICY on suspend
clk: tegra: Add the DFLL as a possible parent of the cclk_g clock
ARM: tegra: Add the DFLL to Tegra124 device tree
ARM: tegra: Enable the DFLL on the Jetson TK1
cpufreq: tegra124: Add device tree bindings
cpufreq: tegra: Rename tegra-cpufreq to tegra20-cpufreq
cpufreq: Add cpufreq driver for Tegra124
ARM: tegra: Add entries for cpufreq on Tegra124
ARM: tegra: enable Tegra124 cpufreq driver by default

  .../bindings/clock/nvidia,tegra124-dfll.txt        |   79 +
  .../bindings/cpufreq/tegra124-cpufreq.txt          |   44 +
  arch/arm/boot/dts/tegra124-jetson-tk1.dts          |   15 +-
  arch/arm/boot/dts/tegra124.dtsi                    |   34 +
  arch/arm/configs/tegra_defconfig                   |    1 +
  arch/arm/mach-tegra/Kconfig                        |    1 +
  drivers/clk/tegra/Makefile                         |    3 +
  drivers/clk/tegra/clk-dfll.c                       | 1755 ++++++++++++++++++++
  drivers/clk/tegra/clk-dfll.h                       |   54 +
  drivers/clk/tegra/clk-tegra-super-gen4.c           |   50 +-
  drivers/clk/tegra/clk-tegra124-dfll-fcpu.c         |  163 ++
  drivers/clk/tegra/clk-tegra124.c                   |   82 +
  drivers/clk/tegra/clk.c                            |   36 +-
  drivers/clk/tegra/clk.h                            |    3 +
  drivers/clk/tegra/cvb.c                            |  133 ++
  drivers/clk/tegra/cvb.h                            |   67 +
  drivers/cpufreq/Kconfig.arm                        |   13 +-
  drivers/cpufreq/Makefile                           |    3 +-
  drivers/cpufreq/tegra124-cpufreq.c                 |  217 +++
  .../cpufreq/{tegra-cpufreq.c => tegra20-cpufreq.c} |    0
  include/dt-bindings/reset/tegra124-car.h           |   11 +
  21 files changed, 2730 insertions(+), 34 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt create mode 100644 Documentation/devicetree/bindings/cpufreq/tegra124-cpufreq.txt create mode 100644 drivers/clk/tegra/clk-dfll.c
create mode 100644 drivers/clk/tegra/clk-dfll.h
create mode 100644 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
create mode 100644 drivers/clk/tegra/cvb.c
create mode 100644 drivers/clk/tegra/cvb.h
create mode 100644 drivers/cpufreq/tegra124-cpufreq.c
rename drivers/cpufreq/{tegra-cpufreq.c => tegra20-cpufreq.c} (100%) create mode 100644 include/dt-bindings/reset/tegra124-car.h

--
2.3.0

Quoted 21 lines · Michael Turquette

Quoting Thierry Reding (2015-03-11 03:07:43)

Quoted 7 lines

Hi Mike,

Have you had a chance to look at these changes to the Tegra clock driver? If you're fine with it, I'd like to take these patches through the Tegra tree because the rest of the series depends on them. I can provide a stable branch in case we need to base other Tegra clock changes on top of this.

Hi Thierry,

Clock patches (and corresponding DT binding descriptions and changes to DTS) look fine to me. Please add:

Acked-by Michael Turquette <[email protected]>

I did have a question about the beahvior of clk_put in one of Mikko's patches but it should not gate this series. I'm just trying to find out if we have a bug in the framework or if the Tegra driver is a special case.

Quoted 5 lines

Also I do not think a stable branch is necessary.

Regards,
Mike

Looks like in the meantime, this has been partially broken by 03bc10ab5b0f "clk: check ->determine/round_rate() return value in clk_calc_new_rates". The highest rates supported by the DFLL clock have 1 in the MSB, so those cannot be entered after the aforementioned patch, as the return value of round_rate is interpreted as an error. Avenues that I can see: 1) revert the above patch 2) restrict the cpu clock rate to those with 0 in the MSB 3) move to 64-bit clock rates.

Hi Mikko,

Quoted 28 lines · On Tue, 14 Apr 2015 14:25:59 +0300 Mikko Perttunen <[email protected]>…

Quoted 21 lines · Michael Turquette

Quoting Thierry Reding (2015-03-11 03:07:43)

Quoted 7 lines

Hi Mike,

Have you had a chance to look at these changes to the Tegra clock driver? If you're fine with it, I'd like to take these patches through the Tegra tree because the rest of the series depends on them. I can provide a stable branch in case we need to base other Tegra clock changes on top of this.

Hi Thierry,

Clock patches (and corresponding DT binding descriptions and changes to DTS) look fine to me. Please add:

Acked-by Michael Turquette <[email protected]>

I did have a question about the beahvior of clk_put in one of Mikko's patches but it should not gate this series. I'm just trying to find out if we have a bug in the framework or if the Tegra driver is a special case.

Quoted 5 lines

Also I do not think a stable branch is necessary.

Regards,
Mike

Quoted 7 lines

Looks like in the meantime, this has been partially broken by 03bc10ab5b0f "clk: check ->determine/round_rate() return value in clk_calc_new_rates". The highest rates supported by the DFLL clock have 1 in the MSB, so those cannot be entered after the aforementioned patch, as the return value of round_rate is interpreted as an error. Avenues that I can see: 1) revert the above patch 2) restrict the cpu clock rate to those with 0 in the MSB 3) move to 64-bit clock rates.

How about changing ->determine_rate() and ->round_rate() prototypes so that they always return 0 or an error code and passing the adjusted_rate as an argument ?

Something like that:

int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long *parent_rate);
int (*determine_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long min_rate, unsigned long max_rate, unsigned long *best_parent_rate, struct clk_hw **best_parent_hw);

I know this implies a lot of changes (in all clock drivers and in the core infrastructure), but I really think we should not mix error codes and clock frequencies (even if we decide to move to a 64 bits rate approach).

Anyway, IMHO the only alternative to this solution is solution #3, because #1 implies re-introducing another bug where ->round_rate()/->determine_rate() are silently ignored, and #2 implies lying about your DFLL capabilities.

Mike, what's your opinion ?

Best Regards,

Boris

Quoted 55 lines

Hi Mikko,

Quoted 28 lines · On Tue, 14 Apr 2015 14:25:59 +0300 Mikko Perttunen <[email protected]>…

Quoted 21 lines · Michael Turquette

Quoting Thierry Reding (2015-03-11 03:07:43)

Quoted 7 lines

Hi Mike,

Have you had a chance to look at these changes to the Tegra clock driver? If you're fine with it, I'd like to take these patches through the Tegra tree because the rest of the series depends on them. I can provide a stable branch in case we need to base other Tegra clock changes on top of this.

Hi Thierry,

Clock patches (and corresponding DT binding descriptions and changes to DTS) look fine to me. Please add:

Acked-by Michael Turquette <[email protected]>

I did have a question about the beahvior of clk_put in one of Mikko's patches but it should not gate this series. I'm just trying to find out if we have a bug in the framework or if the Tegra driver is a special case.

Quoted 5 lines

Also I do not think a stable branch is necessary.

Regards,
Mike

Quoted 7 lines

Looks like in the meantime, this has been partially broken by 03bc10ab5b0f "clk: check ->determine/round_rate() return value in clk_calc_new_rates". The highest rates supported by the DFLL clock have 1 in the MSB, so those cannot be entered after the aforementioned patch, as the return value of round_rate is interpreted as an error. Avenues that I can see: 1) revert the above patch 2) restrict the cpu clock rate to those with 0 in the MSB 3) move to 64-bit clock rates.

How about changing ->determine_rate() and ->round_rate() prototypes so that they always return 0 or an error code and passing the adjusted_rate as an argument ?

Something like that:

int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long *parent_rate);
int (*determine_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long min_rate, unsigned long max_rate, unsigned long *best_parent_rate, struct clk_hw **best_parent_hw);

Quoted 4 lines

I know this implies a lot of changes (in all clock drivers and in the core infrastructure), but I really think we should not mix error codes and clock frequencies (even if we decide to move to a 64 bits rate approach).

This sounds like a good idea, too.

Quoted 7 lines

Anyway, IMHO the only alternative to this solution is solution #3, because #1 implies re-introducing another bug where ->round_rate()/->determine_rate() are silently ignored, and #2 implies lying about your DFLL capabilities.

Yeah, #1 and #2 weren't really meant as realistic options to end up with :)

Quoted 7 lines

Mike, what's your opinion ?

Best Regards,

Boris

Cheers,
Mikko.

Quoting Mikko Perttunen (2015-04-14 12:40:36)

Quoted 61 lines · Boris Brezillon

Quoted 59 lines

Hi Mikko,

Quoted 28 lines · On Tue, 14 Apr 2015 14:25:59 +0300 Mikko Perttunen <[email protected]>…

Quoted 21 lines · Michael Turquette

Quoting Thierry Reding (2015-03-11 03:07:43)

Quoted 7 lines

Hi Mike,

Have you had a chance to look at these changes to the Tegra clock driver? If you're fine with it, I'd like to take these patches through the Tegra tree because the rest of the series depends on them. I can provide a stable branch in case we need to base other Tegra clock changes on top of this.

Hi Thierry,

Clock patches (and corresponding DT binding descriptions and changes to DTS) look fine to me. Please add:

Acked-by Michael Turquette <[email protected]>

I did have a question about the beahvior of clk_put in one of Mikko's patches but it should not gate this series. I'm just trying to find out if we have a bug in the framework or if the Tegra driver is a special case.

Quoted 5 lines

Also I do not think a stable branch is necessary.

Regards,
Mike

Quoted 7 lines

Looks like in the meantime, this has been partially broken by 03bc10ab5b0f "clk: check ->determine/round_rate() return value in clk_calc_new_rates". The highest rates supported by the DFLL clock have 1 in the MSB, so those cannot be entered after the aforementioned patch, as the return value of round_rate is interpreted as an error. Avenues that I can see: 1) revert the above patch 2) restrict the cpu clock rate to those with 0 in the MSB 3) move to 64-bit clock rates.

How about changing ->determine_rate() and ->round_rate() prototypes so that they always return 0 or an error code and passing the adjusted_rate as an argument ?

Something like that:

int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long *parent_rate);
int (*determine_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long min_rate, unsigned long max_rate, unsigned long *best_parent_rate, struct clk_hw **best_parent_hw);

I know this implies a lot of changes (in all clock drivers and in the core infrastructure), but I really think we should not mix error codes and clock frequencies (even if we decide to move to a 64 bits rate approach).

Quoted 1 line

This sounds like a good idea, too.

I've had this idea as well, which is to never return rates but only error codes, and rates are passed by reference like in your example above. Clearly the *best_parent_rate stuff already functions this way. Would be cool to use a programming language that supported complex return types ;-)

Quoted 10 lines

Quoted 8 lines

Anyway, IMHO the only alternative to this solution is solution #3, because #1 implies re-introducing another bug where ->round_rate()/->determine_rate() are silently ignored, and #2 implies lying about your DFLL capabilities.

Yeah, #1 and #2 weren't really meant as realistic options to end up with :)

Yes, seems that we're heading towards #3. In the mean time option #1.5 (the one where we change the round_rate/determine_rate semantics) is probably a good idea and can resolve this issue in the shorter term compared to signed 64-bit rates (and will be necessary anyhow if we use unsigned 64-bit rates).

I'll add this to the high priority todo list since the Tegra EMC stuff won't go for 4.1 but will very likely go for 4.2.

Regards,
Mike

Quoted 11 lines

Quoted 7 lines

Mike, what's your opinion ?

Best Regards,

Boris

Cheers,
Mikko.

Quoting Thierry Reding (2015-03-11 03:07:43)

Quoted 7 lines

Hi Mike,

Have you had a chance to look at these changes to the Tegra clock driver? If you're fine with it, I'd like to take these patches through the Tegra tree because the rest of the series depends on them. I can provide a stable branch in case we need to base other Tegra clock changes on top of this.

Hi Thierry,

Clock patches (and corresponding DT binding descriptions and changes to DTS) look fine to me. Please add:

Acked-by Michael Turquette <[email protected]>

I did have a question about the beahvior of clk_put in one of Mikko's patches but it should not gate this series. I'm just trying to find out if we have a bug in the framework or if the Tegra driver is a special case.

Also I do not think a stable branch is necessary.

Quoted 21 lines

Quoting Thierry Reding (2015-03-11 03:07:43)

Quoted 7 lines

Hi Mike,

Have you had a chance to look at these changes to the Tegra clock driver? If you're fine with it, I'd like to take these patches through the Tegra tree because the rest of the series depends on them. I can provide a stable branch in case we need to base other Tegra clock changes on top of this.

Hi Thierry,

Clock patches (and corresponding DT binding descriptions and changes to DTS) look fine to me. Please add:

Acked-by Michael Turquette <[email protected]>

I did have a question about the beahvior of clk_put in one of Mikko's patches but it should not gate this series. I'm just trying to find out if we have a bug in the framework or if the Tegra driver is a special case.

Quoted 1 line

Also I do not think a stable branch is necessary.

Given how this didn't work at all for v4.1, why don't we try to do this the conventional way for v4.2? What I propose is that I collect all the patches that I've had in these branches into a single branch that I can send you shortly after v4.1-rc1. Then you can pull that into the clock tree when you're happy and I can pull that into the Tegra tree again if needed to resolve dependencies.

One problem that I foresee is that the EMC clock driver relies on some symbols exported by the EMC driver, so it will fail to build on its own if merged into the clock tree. Usually I'd say we can solve this by merging a stable branch from the Tegra tree into the clock tree, but if any of the ARM SoC maintainers then decides that, for whatever reason, the Tegra pull request isn't good enough, you'd need to either redo the clock tree or we slip in the changes via the clock tree anyway.

Perhaps a solution would be to implement stubs for the API exposed by the EMC driver and merge that through the clock tree, which should give us a tree that can be built but be a no-op at runtime. Then we can add the actual code to enable EMC scaling in the Tegra tree by providing a real implementation. That way we only have the Tegra tree depend on the clock tree.

How does that sound?

Thierry