Tegra: Use cpufreq-cpu0 driver

This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.

Most of the platform specific bits are picked from tegra-cpufreq.c.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/clk/tegra/Makefile      |   1 +
 drivers/clk/tegra/clk-cpu.c     | 164 ++++++++++++++++++++++++++++++++++++++++
 drivers/clk/tegra/clk-tegra30.c |   4 +
 include/linux/clk/tegra.h       |   1 +
 4 files changed, 170 insertions(+)
 create mode 100644 drivers/clk/tegra/clk-cpu.c

drivers/clk/tegra/Makefile +1 -0

index f49fac2..0e818c0 100644
@@ -10,3 +10,4 @@ obj-y                                 += clk-super.o
 obj-$(CONFIG_ARCH_TEGRA_2x_SOC)         += clk-tegra20.o
 obj-$(CONFIG_ARCH_TEGRA_3x_SOC)         += clk-tegra30.o
 obj-$(CONFIG_ARCH_TEGRA_114_SOC)       += clk-tegra114.o
+obj-$(CONFIG_GENERIC_CPUFREQ_CPU0)     += clk-cpu.o

drivers/clk/tegra/clk-cpu.c new file +164 -0

new file mode 100644
index 0000000..01716d6
@@ -0,0 +1,164 @@
+/*
+ * Copyright (C) 2013 Linaro
+ *
+ * Author: Viresh Kumar <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+/*
+ * Responsible for setting cpu0 clk as requested by cpufreq-cpu0 driver
+ *
+ * All platform specific bits are taken from tegra-cpufreq driver.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#define to_clk_cpu0(_hw) container_of(_hw, struct clk_cpu0, hw)
+
+struct clk_cpu0 {
+       struct clk_hw hw;
+       spinlock_t *lock;
+};
+
+static struct clk *cpu_clk;
+static struct clk *pll_x_clk;
+static struct clk *pll_p_clk;
+static struct clk *emc_clk;
+
+static unsigned long cpu0_recalc_rate(struct clk_hw *hw,
+               unsigned long parent_rate)
+{
+       return clk_get_rate(cpu_clk);
+}
+
+static long cpu0_round_rate(struct clk_hw *hw, unsigned long drate,
+               unsigned long *parent_rate)
+{
+       return clk_round_rate(cpu_clk, drate);
+}
+
+static int cpu0_set_rate(struct clk_hw *hw, unsigned long rate,
+               unsigned long parent_rate)
+{
+       int ret;
+
+       /*
+        * Vote on memory bus frequency based on cpu frequency
+        * This sets the minimum frequency, display or avp may request higher
+        */
+       if (rate >= 816000000)
+               clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */
+       else if (rate >= 456000000)
+               clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */
+       else
+               clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */
+
+       /*
+        * Take an extra reference to the main pll so it doesn't turn
+        * off when we move the cpu off of it
+        */
+       clk_prepare_enable(pll_x_clk);
+
+       ret = clk_set_parent(cpu_clk, pll_p_clk);
+       if (ret) {
+               pr_err("%s: Failed to switch cpu to clock pll_p\n", __func__);
+               goto out;
+       }
+
+       if (rate == clk_get_rate(pll_p_clk))
+               goto out;
+
+       ret = clk_set_rate(pll_x_clk, rate);
+       if (ret) {
+               pr_err("Failed to change pll_x to %lu\n", rate);
+               goto out;
+       }
+
+       ret = clk_set_parent(cpu_clk, pll_x_clk);
+       if (ret) {
+               pr_err("Failed to switch cpu to clock pll_x\n");
+               goto out;
+       }
+
+out:
+       clk_disable_unprepare(pll_x_clk);
+       return ret;
+}
+
+static struct clk_ops clk_cpu0_ops = {
+       .recalc_rate = cpu0_recalc_rate,
+       .round_rate = cpu0_round_rate,
+       .set_rate = cpu0_set_rate,
+};
+
+struct clk *tegra_clk_register_cpu0(void)
+{
+       struct clk_init_data init;
+       struct clk_cpu0 *cpu0;
+       struct clk *clk;
+
+       cpu0 = kzalloc(sizeof(*cpu0), GFP_KERNEL);
+       if (!cpu0) {
+               pr_err("%s: could not allocate cpu0 clk\n", __func__);
+               return ERR_PTR(-ENOMEM);
+       }
+
+       cpu_clk = clk_get_sys(NULL, "cpu");
+       if (IS_ERR(cpu_clk)) {
+               clk = cpu_clk;
+               goto free_mem;
+       }
+
+       pll_x_clk = clk_get_sys(NULL, "pll_x");
+       if (IS_ERR(pll_x_clk)) {
+               clk = pll_x_clk;
+               goto put_cpu_clk;
+       }
+
+       pll_p_clk = clk_get_sys(NULL, "pll_p_cclk");
+       if (IS_ERR(pll_p_clk)) {
+               clk = pll_p_clk;
+               goto put_pll_x_clk;
+       }
+
+       emc_clk = clk_get_sys("cpu", "emc");
+       if (IS_ERR(emc_clk)) {
+               clk = emc_clk;
+               goto put_pll_p_clk;
+       }
+
+       cpu0->hw.init = &init;
+
+       init.name = "cpu0";
+       init.ops = &clk_cpu0_ops;
+       init.flags = CLK_IS_ROOT | CLK_GET_RATE_NOCACHE;
+       init.num_parents = 0;
+
+       clk = clk_register(NULL, &cpu0->hw);
+       if (!IS_ERR(clk))
+               return clk;
+
+       clk_prepare_enable(emc_clk);
+       clk_prepare_enable(cpu_clk);
+
+       clk_put(emc_clk);
+put_pll_p_clk:
+       clk_put(pll_p_clk);
+put_pll_x_clk:
+       clk_put(pll_x_clk);
+put_cpu_clk:
+       clk_put(cpu_clk);
+free_mem:
+       kfree(cpu0);
+
+       pr_err("%s: clk register failed\n", __func__);
+
+       return NULL;
+}

drivers/clk/tegra/clk-tegra30.c +4 -0

index e2c6ca0..1cabeea 100644
@@ -1396,6 +1396,10 @@ static void __init tegra30_super_clk_init(void)
                                        CLK_SET_RATE_PARENT, 1, 2);
        clk_register_clkdev(clk, "twd", NULL);
        clks[twd] = clk;
+
+       /* cpu0 clk for cpufreq driver */
+       clk = tegra_clk_register_cpu0();
+       clk_register_clkdev(clk, NULL, "cpu0");
 }
 
 static const char *mux_pllacp_clkm[] = { "pll_a_out0", "unused", "pll_p",

include/linux/clk/tegra.h +1 -0

index 23a0cee..d416138 100644
@@ -128,5 +128,6 @@ static inline void tegra_periph_reset_deassert(struct clk *c) {}
 static inline void tegra_periph_reset_assert(struct clk *c) {}
 #endif
 void tegra_clocks_apply_init_table(void);
+struct clk *tegra_clk_register_cpu0(void);
 
 #endif /* __LINUX_CLK_TEGRA_H_ */

Signature · 1 line

1.7.12.rc2.18.g61b472e

This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.

Most of the platform specific bits are picked from tegra-cpufreq.c.

Quoted 4 lines · Viresh Kumar

This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.

Most of the platform specific bits are picked from tegra-cpufreq.c.

Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".

Quoted 10 lines · Stephen Warren

Quoted 4 lines · Viresh Kumar

This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.

Most of the platform specific bits are picked from tegra-cpufreq.c.

Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".

I see it as this: There is a clock in system for cpu, call it "cpu". Now we must be able to provide get/set routines for it. A set should set the frequency to whatever is asked for and should really worry about how that is being set. This part is internal to "cpu" clk.

This is what cpufreq-cpu0 driver should expect and does. Current "cpu" clock implemented doesn't provide this facility ? And so this wrapper made sense to me.

Quoted 12 lines · Viresh Kumar

Quoted 10 lines · Stephen Warren

Quoted 4 lines · Viresh Kumar

This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.

Most of the platform specific bits are picked from tegra-cpufreq.c.

Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".

Quoted 4 lines

I see it as this: There is a clock in system for cpu, call it "cpu". Now we must be able to provide get/set routines for it. A set should set the frequency to whatever is asked for and should really worry about how that is being set. This part is internal to "cpu" clk.

Sure.

Quoted 3 lines

This is what cpufreq-cpu0 driver should expect and does. Current "cpu" clock implemented doesn't provide this facility ? And so this wrapper made sense to me.

But the additional management logic on top of the raw clock is exactly what the cpufreq driver is for. This patch series is basically moving the cpufreq driver code inside the clock code instead.

Quoted 24 lines · Stephen Warren

Quoted 12 lines · Viresh Kumar

Quoted 10 lines · Stephen Warren

Quoted 4 lines · Viresh Kumar

This patch adds CPU0's clk driver for Tegra. It will be used by the generic cpufreq-cpu0 driver to get/set cpu clk.

Most of the platform specific bits are picked from tegra-cpufreq.c.

Hmmm. I'm not sure if it makes sense to represent this as a clock object; isn't this more of a virtual construct that manages the rate of the clock, rather than an actual clock? The actual clock already exists as "cpu".

Quoted 4 lines

I see it as this: There is a clock in system for cpu, call it "cpu". Now we must be able to provide get/set routines for it. A set should set the frequency to whatever is asked for and should really worry about how that is being set. This part is internal to "cpu" clk.

Sure.

Quoted 3 lines

This is what cpufreq-cpu0 driver should expect and does. Current "cpu" clock implemented doesn't provide this facility ? And so this wrapper made sense to me.

Quoted 3 lines

But the additional management logic on top of the raw clock is exactly what the cpufreq driver is for. This patch series is basically moving the cpufreq driver code inside the clock code instead.

Above "sure" didn't go very well with what you wrote here :)

The additional management that we are required to do isn't cpufreq driver specific but cpu or platform specific.

Right, and that's *exactly* what having a cpufreq driver is for; to implement the details of CPU clock management.

Quoted 2 lines · Stephen Warren

Right, and that's *exactly* what having a cpufreq driver is for; to implement the details of CPU clock management.

cpufreq drivers used to keep such information since a long time, probably because there wasn't another place to keep them and provide generic API's (like generic clock framework).. And so this replication started to get in place which we are trying to get rid of now.

All cpufreq drivers share a lot of common code which can go away and so cpufreq-cpu0 was introduced..

With this patchset this replication goes away for tegra atleast at the cost of a platform specific clk-cpu driver.. I think that's a good deal, isn't it?

And that's the only way you can use these generic drivers that we have...

I don't think so. I think it's reasonable to have a per-SoC cpufreq driver whose primary content is the parameterization data and/or custom hooks for a unified core cpufreq driver. The duplicate parts of each cpufreq driver can be moved into the core cpufreq driver, but the non-duplicate parts remain. That's how many other subsystems work (MMC, USB, ASoC spring to mind).

cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.

Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.

arch/arm/boot/dts/tegra114.dtsi +12 -0

index abf6c40..730e0d9 100644
@@ -438,6 +438,18 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a15";
 			reg = <0>;
+			operating-points = <
+				/* kHz    ignored */
+				 216000   1000000
+				 312000   1000000
+				 456000   1000000
+				 608000   1000000
+				 760000   1000000
+				 816000   1000000
+				 912000   1000000
+				 1000000  1000000
+			>;
+			clock-latency = <300000>;
 		};

Quoted 6 lines · Viresh Kumar

cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.

Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.

This patch is problematic w.r.t. DT being an ABI.

We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.

As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.

Similarly, we can't put invalid voltages into the DT, since if a later kernel version starts actually using that field, the HW will no longer work correctly. Unless perhaps we put 0 into the DT and make the binding define that 0 means "you can't change the voltage at all away from the boot value"?

Is the operating-points property documented in
Documentation/devicetree/bindings/ somewhere?

(Also Cc'ing the DT mailing list and maintainers)

Quoted 9 lines · Stephen Warren

Quoted 6 lines · Viresh Kumar

cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.

Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.

This patch is problematic w.r.t. DT being an ABI.

:(

Quoted 4 lines

We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.

To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:

Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt

Or you meant, Tegra never required voltage levels and we are getting them in here.

Quoted 3 lines

As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.

Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.

Btw, you must have some specific voltage level for each freq, we can get them here..

Quoted 24 lines · Viresh Kumar

Quoted 9 lines · Stephen Warren

Quoted 6 lines · Viresh Kumar

cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.

Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.

This patch is problematic w.r.t. DT being an ABI.

:(

Quoted 4 lines

We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.

To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:

Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt

Quoted 2 lines

Or you meant, Tegra never required voltage levels and we are getting them in here.

The current Tegra *.dts files do not contain this property. The current Tegra *.dts files must continue to work without modification in future kernels.

Quoted 7 lines

Quoted 3 lines

As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.

Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.

Sure. That's what the Tegra-specific cpufreq driver should do. It should be the top-level cpufreq driver. If parts of the code can be implemented by library functions or a core parameterizable driver, then presumably the Tegra driver would simply exist to provide those parameters and/or callback functions to the generic driver.

Quoted 2 lines

Btw, you must have some specific voltage level for each freq, we can get them here..

Yes, I'm sure we do, but I have no idea what they are:-( It may even be board-specific or SoC-SKU-specific. I think we should defer this aspect for now.

Quoted 28 lines · Stephen Warren

Quoted 26 lines · Viresh Kumar

Quoted 9 lines · Stephen Warren

Quoted 6 lines · Viresh Kumar

cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.

Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.

This patch is problematic w.r.t. DT being an ABI.

:(

Quoted 4 lines

We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.

To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:

Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt

Or you meant, Tegra never required voltage levels and we are getting them in here.

Quoted 3 lines

The current Tegra *.dts files do not contain this property. The current Tegra *.dts files must continue to work without modification in future kernels.

But that can't be true always.. Specially when we are moving things to DT...

For example, we are moving your DMA driver to DT and hence in the platform code, we are making a new DT node + removing static platform device.

Now, old DT can't work with new kernel... That is just not possible. That statement might be true for cases where we are just upgrading existing DT support (but I doubt it there as well :) )..

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.

Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.

Quoted 5 lines

Sure. That's what the Tegra-specific cpufreq driver should do. It should be the top-level cpufreq driver. If parts of the code can be implemented by library functions or a core parameterizable driver, then presumably the Tegra driver would simply exist to provide those parameters and/or callback functions to the generic driver.

That would be something similar to what we are discussing on other thread about new platform device...

You are asking me to go back to platform specific code instead of DT. When there exists a generic enough way of providing this information via DT, why should we put this in a driver?

Quoted 18 lines · Stephen Warren

Quoted 9 lines · Viresh Kumar

Quoted 6 lines

cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node for multiple SoCs.

Voltage levels aren't used until now for tegra and so a flat value which would eventually be ignored is used to represent voltage.

This patch is problematic w.r.t. DT being an ABI.

:(

Quoted 4 lines

We can certainly add new optional properties to a DT binding that enable new features. However, a new version of a binding can't require new properties to exist that didn't before, since that means that old DTs won't work with new kernels that require the new properties.

Quoted 8 lines

To be honest I didn't get it completely. You meant operating-points wasn't present before? Its here:

Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt Documentation/devicetree/bindings/power/opp.txt

Or you meant, Tegra never required voltage levels and we are getting them in here.

The current Tegra *.dts files do not contain this property. The current Tegra *.dts files must continue to work without modification in future kernels.

Quoted 7 lines

Quoted 3 lines

As such, I believe we do need some Tegra-specific piece of code that defines these OPP tables in the kernel, so that the operating-points property is not needed.

Generic cpufreq driver depends on OPP library and so somebody has to provide them. Now you can do it by calling opp_add() for each OPP you have or otherwise.

Sure. That's what the Tegra-specific cpufreq driver should do. It should be the top-level cpufreq driver. If parts of the code can be implemented by library functions or a core parameterizable driver, then presumably the Tegra driver would simply exist to provide those parameters and/or callback functions to the generic driver.

Quoted 2 lines

Btw, you must have some specific voltage level for each freq, we can get them here..

Yes, I'm sure we do, but I have no idea what they are:-( It may even be board-specific or SoC-SKU-specific. I think we should defer this aspect for now.

From what I learned those voltage levels are dependent on both the Speedo and the process ID of the specific Tegra processor. So you really get a two dimensional mapping table instead of a single OPP. Also you can not scale the CPU voltage on it's own, but have to make sure the core voltage isn't too far away from. Then core voltage also depends on the operating states of engines like GR2D or even display.

Quoted 6 lines

From what I learned those voltage levels are dependent on both the Speedo and the process ID of the specific Tegra processor. So you really get a two dimensional mapping table instead of a single OPP. Also you can not scale the CPU voltage on it's own, but have to make sure the core voltage isn't too far away from. Then core voltage also depends on the operating states of engines like GR2D or even display.

So if they depend on a certain type of SoC, which they should, then we can get these initialized from that SoC's dts/dtsi file instead of a common file.. And so that would resolve the issue you just reported.

Now I haven't proposed in the patch that we will change these voltage levels at all.. This is regulator specific code and would come into play only when regulators are registered for cpu.. Otherwise we will just play with frequency..

Passing OPP instead of just list of frequencies is the generic way this is done now a days..

You can certainly define the mapping table in DT where a specialized Tegra cpufreq driver could read it in and then map frequency to voltage. But that's a runtime decision, as Speedo and process ID are fuse values and can not be represented in DT.

The problem with this is that the hardware description now associates voltages with certain frequencies and even if they are not used by the Linux driver they are plain wrong.

Quoted 4 lines · Lucas Stach

You can certainly define the mapping table in DT where a specialized Tegra cpufreq driver could read it in and then map frequency to voltage. But that's a runtime decision, as Speedo and process ID are fuse values and can not be represented in DT.

Quoted 3 lines

The problem with this is that the hardware description now associates voltages with certain frequencies and even if they are not used by the Linux driver they are plain wrong.

Hmm. I understand.
Then we probably need mach-tegra/opp.c to call opp_add() for all such OPPs.. Neither DT nor cpufreq driver are the right place for this.

cpufreq-cpu0 driver is dependent on OPP library and hence we need to enable it for Tegra as we are going to use cpufreq-cpu0.

Quoted 2 lines · Viresh Kumar

cpufreq-cpu0 driver is dependent on OPP library and hence we need to enable it for Tegra as we are going to use cpufreq-cpu0.

Shouldn't these be selected by something in drivers/cpufreq/Kconfig?

Quoted 6 lines · Viresh Kumar

cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is created for the SoC which wants to use it. Lets create a platform device for cpufreq-cpu0 driver for Tegra.

Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver and hence there will not be any conflicts between two cpufreq drivers.

arch/arm/mach-tegra/tegra.c static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = { · line 85

Quoted 5 lines

arch/arm/mach-tegra/tegra.c +1 -0

 
 static void __init tegra_dt_init(void)
 {
+     struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };

static? const?

Quoted 6 lines

      struct soc_device_attribute *soc_dev_attr;
      struct soc_device *soc_dev;
      struct device *parent = NULL;

      tegra_clocks_apply_init_table();

+ platform_device_register_full(&devinfo);

This seems awfully like going back to board files. Shouldn't something that binds to the CPU nodes register the cpufreq device automatically, based on the CPU's compatible value?

Quoted 20 lines · Stephen Warren

Quoted 18 lines · Viresh Kumar

cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is created for the SoC which wants to use it. Lets create a platform device for cpufreq-cpu0 driver for Tegra.

Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver and hence there will not be any conflicts between two cpufreq drivers.

arch/arm/mach-tegra/tegra.c +2 -0

 
 static void __init tegra_dt_init(void)
 {
+     struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };
      struct soc_device_attribute *soc_dev_attr;
      struct soc_device *soc_dev;
      struct device *parent = NULL;
 
      tegra_clocks_apply_init_table();
+     platform_device_register_full(&devinfo);

Quoted 3 lines

This seems awfully like going back to board files. Shouldn't something that binds to the CPU nodes register the cpufreq device automatically, based on the CPU's compatible value?

This link has got some information why we can't have a node for cpufreq or a compatibility value..

http://permalink.gmane.org/gmane.linux.kernel.cpufreq/9018

That link only describes why we shouldn't have a dedicated compatible value for cpufreq. I certainly agree with that. However, I think it's reasonable that whatever code binds to:

        compatible = "arm,cortex-a9";

... should instantiate any virtual devices that relate to the CPU.

Doing so would be similar to how the Tegra I2S driver instantiates the internal struct device that ASoC needs for the PCM/DMA device, rather than having board-dt-tegra20.c do it, like it would have done in board-file days.

Quoted 4 lines · Stephen Warren

That link only describes why we shouldn't have a dedicated compatible value for cpufreq. I certainly agree with that. However, I think it's reasonable that whatever code binds to:

Quoted 3 lines

        compatible = "arm,cortex-a9";

... should instantiate any virtual devices that relate to the CPU.

But how would we know here if platform really wants us to probe cpufreq-cpu0 driver? On multiplatform kernel there can be multiple cpufreq drivers available and there has to be some sort of code in DT or platform code that reflects which driver we want to use.

Quoted 9 lines · Viresh Kumar

Quoted 7 lines · Stephen Warren

That link only describes why we shouldn't have a dedicated compatible value for cpufreq. I certainly agree with that. However, I think it's reasonable that whatever code binds to:

        compatible = "arm,cortex-a9";

... should instantiate any virtual devices that relate to the CPU.

Quoted 4 lines

But how would we know here if platform really wants us to probe cpufreq-cpu0 driver? On multiplatform kernel there can be multiple cpufreq drivers available and there has to be some sort of code in DT or platform code that reflects which driver we want to use.

Presumably the code would look at the top-level DT node's compatible value (e.g. "nvidia,tegra20").

Hi Stephen,

This is the first attempt to get rid of tegra-cpufreq driver. This patchset tries to add supporting infrastructure for tegra to use cpufreq-cpu0 driver.

Quoted 4 lines · Viresh Kumar

Hi Stephen,

This is the first attempt to get rid of tegra-cpufreq driver. This patchset tries to add supporting infrastructure for tegra to use cpufreq-cpu0 driver.

If tegra has only 4-core fast cpu, I would agree with the patch set. But now I'm not so sure. Tegra cpuquiet driver and cluster switch may need special settings of cpufreq driver.