| Message ID | 1405957142-19416-15-git-send-email-ttynkkynen@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Monday, July 21, 2014 06:39:00 PM Tuomas Tynkkynen wrote: > Add a new cpufreq driver for Tegra124. Instead of using the PLLX as > the CPU clocksource, switch immediately to the DFLL. It allows the use > of higher clock rates, and will automatically scale the CPU voltage as > well. Besides the CPU clocksource switch, we let the cpufreq-cpu0 driver > for all the cpufreq operations. > > This driver also relies on the DFLL driver to fill the OPP table for the > CPU0 device, so that the cpufreq-cpu0 driver knows what frequencies to > use. > > This driver is a completely independent of the old cpufreq driver > (tegra-cpufreq), which is only used on Tegra20. > > Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com> I'm leaving this to Viresh. If he AKCs it for you, please feel free to push this through whatever tree you think is appropriate. > --- > v2 changes: > - use the cpufreq-cpu0 driver > > drivers/cpufreq/Kconfig.arm | 1 + > drivers/cpufreq/Makefile | 1 + > drivers/cpufreq/tegra124-cpufreq.c | 169 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 171 insertions(+) > create mode 100644 drivers/cpufreq/tegra124-cpufreq.c > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index 7364a53..df3c73e 100644 > --- a/drivers/cpufreq/Kconfig.arm > +++ b/drivers/cpufreq/Kconfig.arm > @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ > config ARM_TEGRA_CPUFREQ > bool "TEGRA CPUFreq support" > depends on ARCH_TEGRA > + depends on GENERIC_CPUFREQ_CPU0 > default y > help > This adds the CPUFreq driver support for TEGRA SOCs. > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile > index db6d9a2..3437d24 100644 > --- a/drivers/cpufreq/Makefile > +++ b/drivers/cpufreq/Makefile > @@ -76,6 +76,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_TEGRA_CPUFREQ) += tegra-cpufreq.o > +obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra124-cpufreq.o > obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o > > ################################################################################## > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c > new file mode 100644 > index 0000000..65ff428 > --- /dev/null > +++ b/drivers/cpufreq/tegra124-cpufreq.c > @@ -0,0 +1,169 @@ > +/* > + * 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. > + * > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/clk.h> > +#include <linux/cpufreq.h> > +#include <linux/cpu.h> > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_opp.h> > +#include <linux/types.h> > + > +static struct cpufreq_frequency_table *freq_table; > + > +static struct device *cpu_dev; > +static struct clk *cpu_clk; > +static struct clk *pllp_clk; > +static struct clk *pllx_clk; > +static struct clk *dfll_clk; > + > +static int tegra124_cpu_switch_to_dfll(void) > +{ > + struct clk *original_cpu_clk_parent; > + unsigned long rate; > + struct dev_pm_opp *opp; > + int ret; > + > + rate = clk_get_rate(cpu_clk); > + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); > + if (IS_ERR(opp)) > + return PTR_ERR(opp); > + > + ret = clk_set_rate(dfll_clk, rate); > + if (ret) > + return ret; > + > + original_cpu_clk_parent = clk_get_parent(cpu_clk); > + clk_set_parent(cpu_clk, pllp_clk); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(dfll_clk); > + if (ret) > + goto out_switch_to_original_parent; > + > + clk_set_parent(cpu_clk, dfll_clk); > + > + return 0; > + > +out_switch_to_original_parent: > + clk_set_parent(cpu_clk, original_cpu_clk_parent); > + > + return ret; > +} > + > +static struct platform_device_info cpufreq_cpu0_devinfo = { > + .name = "cpufreq-cpu0", > +}; > + > +static int tegra124_cpufreq_probe(struct platform_device *pdev) > +{ > + int ret; > + > + cpu_dev = get_cpu_device(0); > + if (!cpu_dev) > + return -ENODEV; > + > + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g"); > + if (IS_ERR(cpu_clk)) > + return PTR_ERR(cpu_clk); > + > + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll"); > + if (IS_ERR(dfll_clk)) { > + ret = PTR_ERR(dfll_clk); > + goto out_put_cpu_clk; > + } > + > + pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x"); > + if (IS_ERR(pllx_clk)) { > + ret = PTR_ERR(pllx_clk); > + goto out_put_dfll_clk; > + } > + > + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p"); > + if (IS_ERR(pllp_clk)) { > + ret = PTR_ERR(pllp_clk); > + goto out_put_pllx_clk; > + } > + > + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); > + if (ret) > + goto out_put_pllp_clk; > + > + ret = tegra124_cpu_switch_to_dfll(); > + if (ret) > + goto out_free_table; > + > + platform_device_register_full(&cpufreq_cpu0_devinfo); > + > + return 0; > + > +out_free_table: > + dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); > +out_put_pllp_clk: > + clk_put(pllp_clk); > +out_put_pllx_clk: > + clk_put(pllx_clk); > +out_put_dfll_clk: > + clk_put(dfll_clk); > +out_put_cpu_clk: > + clk_put(cpu_clk); > + > + return ret; > +} > + > +static struct platform_driver tegra124_cpufreq_platdrv = { > + .driver = { > + .name = "cpufreq-tegra124", > + .owner = THIS_MODULE, > + }, > + .probe = tegra124_cpufreq_probe, > +}; > + > +static const struct of_device_id soc_of_matches[] = { > + { .compatible = "nvidia,tegra124", }, > + {} > +}; > + > +static int __init tegra_cpufreq_init(void) > +{ > + int ret; > + struct platform_device *pdev; > + > + if (!of_find_matching_node(NULL, soc_of_matches)) > + return -ENODEV; > + > + ret = platform_driver_register(&tegra124_cpufreq_platdrv); > + if (ret) > + return ret; > + > + pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); > + if (IS_ERR(pdev)) { > + platform_driver_unregister(&tegra124_cpufreq_platdrv); > + return PTR_ERR(pdev); > + } > + > + return 0; > +} > + > +MODULE_AUTHOR("Tuomas Tynkkynen <ttynkkynen@nvidia.com>"); > +MODULE_DESCRIPTION("cpufreq driver for nVIDIA Tegra124"); > +MODULE_LICENSE("GPLv2"); > +module_init(tegra_cpufreq_init); >
On 21 July 2014 21:09, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index 7364a53..df3c73e 100644 > --- a/drivers/cpufreq/Kconfig.arm > +++ b/drivers/cpufreq/Kconfig.arm > @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ > config ARM_TEGRA_CPUFREQ > bool "TEGRA CPUFreq support" > depends on ARCH_TEGRA > + depends on GENERIC_CPUFREQ_CPU0 Wouldn't this also disturb the existing cpufreq driver for earlier tegra platforms? i.e. we don't need cpufreq-cpu0 for them atleast as of now. > default y > help > This adds the CPUFreq driver support for TEGRA SOCs. > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile > index db6d9a2..3437d24 100644 > --- a/drivers/cpufreq/Makefile > +++ b/drivers/cpufreq/Makefile > @@ -76,6 +76,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_TEGRA_CPUFREQ) += tegra-cpufreq.o > +obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra124-cpufreq.o Maybe, you can update the same line if you want. > obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o > > ################################################################################## > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c > +static struct cpufreq_frequency_table *freq_table; > + > +static struct device *cpu_dev; > +static struct clk *cpu_clk; > +static struct clk *pllp_clk; > +static struct clk *pllx_clk; > +static struct clk *dfll_clk; The routines in this file are going to be called just once at boot, right? In that case we are actually wasting some memory by creating globals. Probably just move all these in a struct and allocate it at runtime. > +static int tegra124_cpu_switch_to_dfll(void) > +{ > + struct clk *original_cpu_clk_parent; > + unsigned long rate; > + struct dev_pm_opp *opp; > + int ret; > + > + rate = clk_get_rate(cpu_clk); > + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); > + if (IS_ERR(opp)) > + return PTR_ERR(opp); > + > + ret = clk_set_rate(dfll_clk, rate); > + if (ret) > + return ret; > + > + original_cpu_clk_parent = clk_get_parent(cpu_clk); > + clk_set_parent(cpu_clk, pllp_clk); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(dfll_clk); > + if (ret) > + goto out_switch_to_original_parent; > + > + clk_set_parent(cpu_clk, dfll_clk); > + > + return 0; > + > +out_switch_to_original_parent: > + clk_set_parent(cpu_clk, original_cpu_clk_parent); > + > + return ret; > +} > + > +static struct platform_device_info cpufreq_cpu0_devinfo = { > + .name = "cpufreq-cpu0", > +}; > + > +static int tegra124_cpufreq_probe(struct platform_device *pdev) > +{ > + int ret; > + > + cpu_dev = get_cpu_device(0); > + if (!cpu_dev) > + return -ENODEV; > + Shouldn't we do a of_node_get() here? > + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g"); > + if (IS_ERR(cpu_clk)) > + return PTR_ERR(cpu_clk); > + > + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll"); > + if (IS_ERR(dfll_clk)) { > + ret = PTR_ERR(dfll_clk); > + goto out_put_cpu_clk; > + } > + > + pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x"); > + if (IS_ERR(pllx_clk)) { > + ret = PTR_ERR(pllx_clk); > + goto out_put_dfll_clk; > + } > + > + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p"); > + if (IS_ERR(pllp_clk)) { > + ret = PTR_ERR(pllp_clk); > + goto out_put_pllx_clk; > + } > + > + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); > + if (ret) > + goto out_put_pllp_clk; Why do you need this? cpufreq-cpu0 also does it and this freq_table is just not getting used at all then. > + > + ret = tegra124_cpu_switch_to_dfll(); > + if (ret) > + goto out_free_table; > + > + platform_device_register_full(&cpufreq_cpu0_devinfo); > + > + return 0; > + > +out_free_table: > + dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); > +out_put_pllp_clk: > + clk_put(pllp_clk); > +out_put_pllx_clk: > + clk_put(pllx_clk); > +out_put_dfll_clk: > + clk_put(dfll_clk); > +out_put_cpu_clk: > + clk_put(cpu_clk); > + > + return ret; > +} > + > +static struct platform_driver tegra124_cpufreq_platdrv = { > + .driver = { > + .name = "cpufreq-tegra124", > + .owner = THIS_MODULE, > + }, > + .probe = tegra124_cpufreq_probe, > +}; > + > +static const struct of_device_id soc_of_matches[] = { > + { .compatible = "nvidia,tegra124", }, > + {} > +}; > + > +static int __init tegra_cpufreq_init(void) > +{ > + int ret; > + struct platform_device *pdev; > + > + if (!of_find_matching_node(NULL, soc_of_matches)) > + return -ENODEV; > + > + ret = platform_driver_register(&tegra124_cpufreq_platdrv); > + if (ret) > + return ret; > + > + pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); > + if (IS_ERR(pdev)) { > + platform_driver_unregister(&tegra124_cpufreq_platdrv); > + return PTR_ERR(pdev); > + } Why create another unnecessary platform-device/driver here? If of_find_matching_node() passes and you really need to probe cpufreq-cpu0 here, then just remove the other two calls and move probe's implementation here only. > + return 0; > +} > + > +MODULE_AUTHOR("Tuomas Tynkkynen <ttynkkynen@nvidia.com>"); > +MODULE_DESCRIPTION("cpufreq driver for nVIDIA Tegra124"); > +MODULE_LICENSE("GPLv2"); > +module_init(tegra_cpufreq_init); > -- > 1.8.1.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote: > On 21 July 2014 21:09, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: > > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > > index 7364a53..df3c73e 100644 > > --- a/drivers/cpufreq/Kconfig.arm > > +++ b/drivers/cpufreq/Kconfig.arm > > @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ > > config ARM_TEGRA_CPUFREQ > > bool "TEGRA CPUFreq support" > > depends on ARCH_TEGRA > > + depends on GENERIC_CPUFREQ_CPU0 > > Wouldn't this also disturb the existing cpufreq driver for earlier > tegra platforms? i.e. we don't need cpufreq-cpu0 for them > atleast as of now. Perhaps this should be "select" rather than "depends on"? > > +static int tegra124_cpufreq_probe(struct platform_device *pdev) > > +{ > > + int ret; > > + > > + cpu_dev = get_cpu_device(0); > > + if (!cpu_dev) > > + return -ENODEV; > > + > > Shouldn't we do a of_node_get() here? I think this would need to be get_device() since it's the struct device that's being used subsequently. Thierry
On 23 July 2014 12:24, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote: >> On 21 July 2014 21:09, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: >> >> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm >> > index 7364a53..df3c73e 100644 >> > --- a/drivers/cpufreq/Kconfig.arm >> > +++ b/drivers/cpufreq/Kconfig.arm >> > @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ >> > config ARM_TEGRA_CPUFREQ >> > bool "TEGRA CPUFreq support" >> > depends on ARCH_TEGRA >> > + depends on GENERIC_CPUFREQ_CPU0 >> >> Wouldn't this also disturb the existing cpufreq driver for earlier >> tegra platforms? i.e. we don't need cpufreq-cpu0 for them >> atleast as of now. > > Perhaps this should be "select" rather than "depends on"? Don't know, its not optionaly for tegra124 and so a "depends on" might fit better ? >> > +static int tegra124_cpufreq_probe(struct platform_device *pdev) >> > +{ >> > + int ret; >> > + >> > + cpu_dev = get_cpu_device(0); >> > + if (!cpu_dev) >> > + return -ENODEV; >> > + >> >> Shouldn't we do a of_node_get() here? > > I think this would need to be get_device() since it's the struct device > that's being used subsequently. Probably I didn't write it well.. What I meant was after doing a get_cpu_device() we might also need to do of_node_get(cpu_dev->of_node) as we would be using of_node in further code. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote: [...] > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c [...] > +static int tegra124_cpu_switch_to_dfll(void) > +{ > + struct clk *original_cpu_clk_parent; Maybe just "parent"? > + unsigned long rate; > + struct dev_pm_opp *opp; > + int ret; > + > + rate = clk_get_rate(cpu_clk); > + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); > + if (IS_ERR(opp)) > + return PTR_ERR(opp); > + > + ret = clk_set_rate(dfll_clk, rate); > + if (ret) > + return ret; > + > + original_cpu_clk_parent = clk_get_parent(cpu_clk); > + clk_set_parent(cpu_clk, pllp_clk); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(dfll_clk); > + if (ret) > + goto out_switch_to_original_parent; This could simply be "out" or "err" or anything else shorter than the above. > + > + clk_set_parent(cpu_clk, dfll_clk); > + > + return 0; > + > +out_switch_to_original_parent: > + clk_set_parent(cpu_clk, original_cpu_clk_parent); > + > + return ret; > +} > + > +static struct platform_device_info cpufreq_cpu0_devinfo = { > + .name = "cpufreq-cpu0", > +}; > + > +static int tegra124_cpufreq_probe(struct platform_device *pdev) > +{ > + int ret; > + > + cpu_dev = get_cpu_device(0); > + if (!cpu_dev) > + return -ENODEV; > + > + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g"); > + if (IS_ERR(cpu_clk)) > + return PTR_ERR(cpu_clk); > + > + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll"); > + if (IS_ERR(dfll_clk)) { > + ret = PTR_ERR(dfll_clk); > + goto out_put_cpu_clk; > + } > + > + pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x"); > + if (IS_ERR(pllx_clk)) { > + ret = PTR_ERR(pllx_clk); > + goto out_put_dfll_clk; > + } > + > + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p"); > + if (IS_ERR(pllp_clk)) { > + ret = PTR_ERR(pllp_clk); > + goto out_put_pllx_clk; > + } Can the above not be devm_clk_get(cpu_dev, "...") so that you can remove all the clk_put() calls in the cleanup code below? > + > + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); > + if (ret) > + goto out_put_pllp_clk; > + > + ret = tegra124_cpu_switch_to_dfll(); > + if (ret) > + goto out_free_table; > + > + platform_device_register_full(&cpufreq_cpu0_devinfo); Should the cpufreq_cpu0_devinfo device perhaps be a child of pdev? > + > + return 0; > + > +out_free_table: > + dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); > +out_put_pllp_clk: > + clk_put(pllp_clk); > +out_put_pllx_clk: > + clk_put(pllx_clk); > +out_put_dfll_clk: > + clk_put(dfll_clk); > +out_put_cpu_clk: > + clk_put(cpu_clk); > + > + return ret; > +} > + > +static struct platform_driver tegra124_cpufreq_platdrv = { > + .driver = { > + .name = "cpufreq-tegra124", > + .owner = THIS_MODULE, > + }, > + .probe = tegra124_cpufreq_probe, Note that simply leaving out .remove() here doesn't guarantee that the driver won't be unloaded. Also building it into the kernel doesn't prevent that. You can still unbind the driver via sysfs. So you'd need to add a .suppress_bind_attrs = true above. But is there even a reason why we need that? Couldn't we make the driver's .remove() undo what .probe() did so that the driver can be unloaded? Otherwise it probably makes more sense not to use a driver (and dummy device) at all as Viresh already mentioned. > +}; > + > +static const struct of_device_id soc_of_matches[] = { > + { .compatible = "nvidia,tegra124", }, > + {} > +}; > + > +static int __init tegra_cpufreq_init(void) > +{ > + int ret; > + struct platform_device *pdev; > + > + if (!of_find_matching_node(NULL, soc_of_matches)) > + return -ENODEV; I think this could be of_machine_is_compatible() since there's only a single entry in the match table. If there's a good chance that we may end up with more entries, perhaps now would be a good time to add an of_match_machine() function? > + > + ret = platform_driver_register(&tegra124_cpufreq_platdrv); > + if (ret) > + return ret; > + > + pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); > + if (IS_ERR(pdev)) { > + platform_driver_unregister(&tegra124_cpufreq_platdrv); > + return PTR_ERR(pdev); > + } > + > + return 0; > +} > + > +MODULE_AUTHOR("Tuomas Tynkkynen <ttynkkynen@nvidia.com>"); > +MODULE_DESCRIPTION("cpufreq driver for nVIDIA Tegra124"); We use "NVIDIA" everywhere nowadays. > +MODULE_LICENSE("GPLv2"); The correct license string is "GPL v2". > +module_init(tegra_cpufreq_init); The placement of this is unusual. It should go immediately below the tegra_cpufreq_init() function. Thierry
On Mon, Jul 21, 2014 at 9:09 PM, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: > Add a new cpufreq driver for Tegra124. Instead of using the PLLX as <snip> > + > +static int tegra124_cpu_switch_to_dfll(void) > +{ > + struct clk *original_cpu_clk_parent; > + unsigned long rate; > + struct dev_pm_opp *opp; > + int ret; > + > + rate = clk_get_rate(cpu_clk); > + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); > + if (IS_ERR(opp)) > + return PTR_ERR(opp); > + > + ret = clk_set_rate(dfll_clk, rate); > + if (ret) > + return ret; > + > + original_cpu_clk_parent = clk_get_parent(cpu_clk); > + clk_set_parent(cpu_clk, pllp_clk); Needs return check. > + if (ret) > + return ret; Which 'ret' is being checked here? nothing is assigned here. May be fixing previous comment will fix this. > + > + ret = clk_prepare_enable(dfll_clk); <snip> > Please read the FAQ at http://www.tux.org/lkml/
On Wed, Jul 23, 2014 at 12:28:21PM +0530, Viresh Kumar wrote: > On 23 July 2014 12:24, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote: > >> On 21 July 2014 21:09, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: > >> > >> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > >> > index 7364a53..df3c73e 100644 > >> > --- a/drivers/cpufreq/Kconfig.arm > >> > +++ b/drivers/cpufreq/Kconfig.arm > >> > @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ > >> > config ARM_TEGRA_CPUFREQ > >> > bool "TEGRA CPUFreq support" > >> > depends on ARCH_TEGRA > >> > + depends on GENERIC_CPUFREQ_CPU0 > >> > >> Wouldn't this also disturb the existing cpufreq driver for earlier > >> tegra platforms? i.e. we don't need cpufreq-cpu0 for them > >> atleast as of now. > > > > Perhaps this should be "select" rather than "depends on"? > > Don't know, its not optionaly for tegra124 and so a "depends on" > might fit better ? ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the Tegra cpufreq driver is enabled. This is mostly just out of convenience, though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so a select will automatically pull in the necessary dependency. With a "depends on" the Tegra cpufreq driver only becomes available after you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive. To illustrate with an example: as a user, I want to enable CPU frequency scaling on Tegra. So I use menuconfig to navigate to the "CPU Frequency scaling" menu (enable it if not available yet) and look for an entry that says "Tegra". But I can't find it because it's hidden due to the lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a generic driver is an implementation detail that users shouldn't have to be aware of. > >> > +static int tegra124_cpufreq_probe(struct platform_device *pdev) > >> > +{ > >> > + int ret; > >> > + > >> > + cpu_dev = get_cpu_device(0); > >> > + if (!cpu_dev) > >> > + return -ENODEV; > >> > + > >> > >> Shouldn't we do a of_node_get() here? > > > > I think this would need to be get_device() since it's the struct device > > that's being used subsequently. > > Probably I didn't write it well.. > > What I meant was after doing a get_cpu_device() we might also need > to do of_node_get(cpu_dev->of_node) as we would be using of_node > in further code. But we're using cpu_dev->of_node, so we need to make sure cpu_dev doesn't go away suddenly. Simply keeping a reference to ->of_node won't ensure that. I guess technically it would be better if get_cpu_device() already incremented the reference count on the returned struct device. Currently it would theoretically still be possible for the device to disappear between the call to get_cpu_device() and a call to get_device(). Thierry
On 23 July 2014 12:54, Thierry Reding <thierry.reding@gmail.com> wrote: > ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the > Tegra cpufreq driver is enabled. This is mostly just out of convenience, > though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so > a select will automatically pull in the necessary dependency. With a Not necessarily. cpufreq-cpu0 can have few unmet dependency. And so there are chances that tegra driver is compiled but cpufreq-cpu0 isn't as we didn't mention it as a *hard* dependency. And so at boot, there wouldn't be any cpufreq support even when tegra's cpufreq driver is available. Though, menuconfig may give some warnings no such situations. > "depends on" the Tegra cpufreq driver only becomes available after > you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive. > > To illustrate with an example: as a user, I want to enable CPU frequency > scaling on Tegra. So I use menuconfig to navigate to the "CPU Frequency > scaling" menu (enable it if not available yet) and look for an entry > that says "Tegra". But I can't find it because it's hidden due to the > lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a > generic driver is an implementation detail that users shouldn't have to > be aware of. Don't know, the guy compiling out stuff should be knowledgeable enough to have a look why tegra cpufreq entry isn't shown in menu. As, probably the above problem I mentioned looks to be of more significance than this one, atleast to me :) And, another thing to mention is that CONFIG_TEGRA_CPUFREQ is valid for earlier platforms as well and so a select/depends wouldn't be valid for earlier platforms. We probably need another Kconfig entry here. > But we're using cpu_dev->of_node, so we need to make sure cpu_dev > doesn't go away suddenly. Simply keeping a reference to ->of_node > won't ensure that. Oh, yeah I completely agree, but don't see that as a normal code style people follow. Probably they take cpu for granted, which doesn't look right :) > I guess technically it would be better if get_cpu_device() already > incremented the reference count on the returned struct device. Currently > it would theoretically still be possible for the device to disappear > between the call to get_cpu_device() and a call to get_device(). I agree again. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/07/14 07:44, Viresh Kumar wrote: > On 21 July 2014 21:09, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: > >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm >> index 7364a53..df3c73e 100644 >> --- a/drivers/cpufreq/Kconfig.arm >> +++ b/drivers/cpufreq/Kconfig.arm >> @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ >> config ARM_TEGRA_CPUFREQ >> bool "TEGRA CPUFreq support" >> depends on ARCH_TEGRA >> + depends on GENERIC_CPUFREQ_CPU0 > > Wouldn't this also disturb the existing cpufreq driver for earlier > tegra platforms? i.e. we don't need cpufreq-cpu0 for them > atleast as of now. > >> default y >> help >> This adds the CPUFreq driver support for TEGRA SOCs. >> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile >> index db6d9a2..3437d24 100644 >> --- a/drivers/cpufreq/Makefile >> +++ b/drivers/cpufreq/Makefile >> @@ -76,6 +76,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_TEGRA_CPUFREQ) += tegra-cpufreq.o >> +obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra124-cpufreq.o > > Maybe, you can update the same line if you want. Oh, right. > >> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o >> >> ################################################################################## >> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c > >> +static struct cpufreq_frequency_table *freq_table; >> + >> +static struct device *cpu_dev; >> +static struct clk *cpu_clk; >> +static struct clk *pllp_clk; >> +static struct clk *pllx_clk; >> +static struct clk *dfll_clk; > > The routines in this file are going to be called just once at boot, right? > In that case we are actually wasting some memory by creating globals. > Probably just move all these in a struct and allocate it at runtime. > Sure. [...] >> + >> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); >> + if (ret) >> + goto out_put_pllp_clk; > > Why do you need this? cpufreq-cpu0 also does it and this freq_table is > just not getting used at all then. > Oops, yes I forgot to remove that part when making the conversion. >> + >> + pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); >> + if (IS_ERR(pdev)) { >> + platform_driver_unregister(&tegra124_cpufreq_platdrv); >> + return PTR_ERR(pdev); >> + } > > Why create another unnecessary platform-device/driver here? If > of_find_matching_node() passes and you really need to probe cpufreq-cpu0 > here, then just remove the other two calls and move probe's implementation > here only. > The platform device is required for the deferred probe that can happen if the DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to respect -EPROBE_DEFER. [...]
On 23/07/14 10:09, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote: > [...] >> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c > [...] >> +static int tegra124_cpu_switch_to_dfll(void) >> +{ >> + struct clk *original_cpu_clk_parent; > > Maybe just "parent"? > >> + unsigned long rate; >> + struct dev_pm_opp *opp; >> + int ret; >> + >> + rate = clk_get_rate(cpu_clk); >> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); >> + if (IS_ERR(opp)) >> + return PTR_ERR(opp); >> + >> + ret = clk_set_rate(dfll_clk, rate); >> + if (ret) >> + return ret; >> + >> + original_cpu_clk_parent = clk_get_parent(cpu_clk); >> + clk_set_parent(cpu_clk, pllp_clk); >> + if (ret) >> + return ret; >> + >> + ret = clk_prepare_enable(dfll_clk); >> + if (ret) >> + goto out_switch_to_original_parent; > > This could simply be "out" or "err" or anything else shorter than the > above. > >> + >> + clk_set_parent(cpu_clk, dfll_clk); >> + >> + return 0; >> + >> +out_switch_to_original_parent: >> + clk_set_parent(cpu_clk, original_cpu_clk_parent); >> + >> + return ret; >> +} >> + >> +static struct platform_device_info cpufreq_cpu0_devinfo = { >> + .name = "cpufreq-cpu0", >> +}; >> + >> +static int tegra124_cpufreq_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + >> + cpu_dev = get_cpu_device(0); >> + if (!cpu_dev) >> + return -ENODEV; >> + >> + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g"); >> + if (IS_ERR(cpu_clk)) >> + return PTR_ERR(cpu_clk); >> + >> + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll"); >> + if (IS_ERR(dfll_clk)) { >> + ret = PTR_ERR(dfll_clk); >> + goto out_put_cpu_clk; >> + } >> + >> + pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x"); >> + if (IS_ERR(pllx_clk)) { >> + ret = PTR_ERR(pllx_clk); >> + goto out_put_dfll_clk; >> + } >> + >> + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p"); >> + if (IS_ERR(pllp_clk)) { >> + ret = PTR_ERR(pllp_clk); >> + goto out_put_pllx_clk; >> + } > > Can the above not be devm_clk_get(cpu_dev, "...") so that you can remove > all the clk_put() calls in the cleanup code below? That would allocate the clks under the cpu_dev's devres list, i.e. all the clk_puts wouldn't happen when the cpufreq driver goes away, but only when cpu_dev itself goes away. > >> + >> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); >> + if (ret) >> + goto out_put_pllp_clk; >> + >> + ret = tegra124_cpu_switch_to_dfll(); >> + if (ret) >> + goto out_free_table; >> + >> + platform_device_register_full(&cpufreq_cpu0_devinfo); > > Should the cpufreq_cpu0_devinfo device perhaps be a child of pdev? Yeah, I suppose it should. >> + >> + return 0; >> + >> +out_free_table: >> + dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); >> +out_put_pllp_clk: >> + clk_put(pllp_clk); >> +out_put_pllx_clk: >> + clk_put(pllx_clk); >> +out_put_dfll_clk: >> + clk_put(dfll_clk); >> +out_put_cpu_clk: >> + clk_put(cpu_clk); >> + >> + return ret; >> +} >> + >> +static struct platform_driver tegra124_cpufreq_platdrv = { >> + .driver = { >> + .name = "cpufreq-tegra124", >> + .owner = THIS_MODULE, >> + }, >> + .probe = tegra124_cpufreq_probe, > > Note that simply leaving out .remove() here doesn't guarantee that the > driver won't be unloaded. Also building it into the kernel doesn't > prevent that. You can still unbind the driver via sysfs. So you'd need > to add a .suppress_bind_attrs = true above. I hadn't heard about suppress_bind_attrs before, it indeed sounds useful. > But is there even a reason why we need that? Couldn't we make the > driver's .remove() undo what .probe() did so that the driver can be > unloaded? I guess that could be done, though to fully undo everything the regulator voltage would also need to be saved/restored. > Otherwise it probably makes more sense not to use a driver (and dummy > device) at all as Viresh already mentioned. > The dummy platform device is only required for probe deferral, if that could be solved in a different way then yes. >> +}; >> + >> +static const struct of_device_id soc_of_matches[] = { >> + { .compatible = "nvidia,tegra124", }, >> + {} >> +}; >> + >> +static int __init tegra_cpufreq_init(void) >> +{ >> + int ret; >> + struct platform_device *pdev; >> + >> + if (!of_find_matching_node(NULL, soc_of_matches)) >> + return -ENODEV; > > I think this could be of_machine_is_compatible() since there's only a > single entry in the match table. If there's a good chance that we may > end up with more entries, perhaps now would be a good time to add an > of_match_machine() function? I think this driver should work on Tegra132 without modifications. of_match_machine() does sound useful for some of the other cpufreq drivers as well and likely for your soc_is_tegra() from the PMC series as well. > >> + >> + ret = platform_driver_register(&tegra124_cpufreq_platdrv); >> + if (ret) >> + return ret; >> + >> + pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); >> + if (IS_ERR(pdev)) { >> + platform_driver_unregister(&tegra124_cpufreq_platdrv); >> + return PTR_ERR(pdev); >> + } >> + >> + return 0; >> +} >> + >> +MODULE_AUTHOR("Tuomas Tynkkynen <ttynkkynen@nvidia.com>"); >> +MODULE_DESCRIPTION("cpufreq driver for nVIDIA Tegra124"); > > We use "NVIDIA" everywhere nowadays. > >> +MODULE_LICENSE("GPLv2"); > > The correct license string is "GPL v2". > >> +module_init(tegra_cpufreq_init); > > The placement of this is unusual. It should go immediately below the > tegra_cpufreq_init() function. > Ok. Thanks, Tuomas
On Wed, Jul 23, 2014 at 01:55:57PM +0530, Viresh Kumar wrote: > On 23 July 2014 12:54, Thierry Reding <thierry.reding@gmail.com> wrote: > > ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the > > Tegra cpufreq driver is enabled. This is mostly just out of convenience, > > though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so > > a select will automatically pull in the necessary dependency. With a > > Not necessarily. cpufreq-cpu0 can have few unmet dependency. And so > there are chances that tegra driver is compiled but cpufreq-cpu0 isn't as > we didn't mention it as a *hard* dependency. > > And so at boot, there wouldn't be any cpufreq support even when tegra's > cpufreq driver is available. > > Though, menuconfig may give some warnings no such situations. > > > "depends on" the Tegra cpufreq driver only becomes available after > > you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive. > > > > To illustrate with an example: as a user, I want to enable CPU frequency > > scaling on Tegra. So I use menuconfig to navigate to the "CPU Frequency > > scaling" menu (enable it if not available yet) and look for an entry > > that says "Tegra". But I can't find it because it's hidden due to the > > lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a > > generic driver is an implementation detail that users shouldn't have to > > be aware of. > > Don't know, the guy compiling out stuff should be knowledgeable enough to > have a look why tegra cpufreq entry isn't shown in menu. As, probably the > above problem I mentioned looks to be of more significance than this one, > atleast to me :) > > And, another thing to mention is that CONFIG_TEGRA_CPUFREQ is valid > for earlier platforms as well and so a select/depends wouldn't be valid for > earlier platforms. We probably need another Kconfig entry here. Yes, sounds like a new Kconfig entry for this specific driver would be a better approach and should remove all the above concerns. Thierry
On Wed, Jul 23, 2014 at 03:35:46PM +0300, Tuomas Tynkkynen wrote: > On 23/07/14 10:09, Thierry Reding wrote: > > On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote: [...] > >> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c [...] > >> + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g"); > >> + if (IS_ERR(cpu_clk)) > >> + return PTR_ERR(cpu_clk); [...] > >> + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p"); > >> + if (IS_ERR(pllp_clk)) { > >> + ret = PTR_ERR(pllp_clk); > >> + goto out_put_pllx_clk; > >> + } > > > > Can the above not be devm_clk_get(cpu_dev, "...") so that you can remove > > all the clk_put() calls in the cleanup code below? > > That would allocate the clks under the cpu_dev's devres list, i.e. all the > clk_puts wouldn't happen when the cpufreq driver goes away, but only when > cpu_dev itself goes away. I don't think so. devres_release_all() is called on driver detach as well. > > But is there even a reason why we need that? Couldn't we make the > > driver's .remove() undo what .probe() did so that the driver can be > > unloaded? > > I guess that could be done, though to fully undo everything the regulator > voltage would also need to be saved/restored. That would certainly be my prefered approach. that way the driver can simply be unloaded, leaving the CPU in the same state as it was after boot. > > Otherwise it probably makes more sense not to use a driver (and dummy > > device) at all as Viresh already mentioned. > > > > The dummy platform device is only required for probe deferral, if that > could be solved in a different way then yes. I don't think it can. Probe deferral is pretty closely tied to devices so it's unlikely to ever get implemented for regular initcalls. And in this case I really think making the driver removable is a good thing. > >> +}; > >> + > >> +static const struct of_device_id soc_of_matches[] = { > >> + { .compatible = "nvidia,tegra124", }, > >> + {} > >> +}; > >> + > >> +static int __init tegra_cpufreq_init(void) > >> +{ > >> + int ret; > >> + struct platform_device *pdev; > >> + > >> + if (!of_find_matching_node(NULL, soc_of_matches)) > >> + return -ENODEV; > > > > I think this could be of_machine_is_compatible() since there's only a > > single entry in the match table. If there's a good chance that we may > > end up with more entries, perhaps now would be a good time to add an > > of_match_machine() function? > > I think this driver should work on Tegra132 without modifications. > of_match_machine() does sound useful for some of the other cpufreq > drivers as well and likely for your soc_is_tegra() from the PMC > series as well. Yes, indeed. I'll give it a shot if you don't beat me to it with this series. Thierry
On 23 July 2014 17:27, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: > The platform device is required for the deferred probe that can happen if the > DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to > respect -EPROBE_DEFER. Oh, which call in this file will return EPROBE_DEFER? I couldn't make out which one will depend on DFLL driver. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/07/14 19:50, Viresh Kumar wrote: > On 23 July 2014 17:27, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: >> The platform device is required for the deferred probe that can happen if the >> DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to >> respect -EPROBE_DEFER. > > Oh, which call in this file will return EPROBE_DEFER? I couldn't make out > which one will depend on DFLL driver. > It's this: +static int tegra124_cpufreq_probe(struct platform_device *pdev) +{ [...] + + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll"); + if (IS_ERR(dfll_clk)) { + ret = PTR_ERR(dfll_clk); + goto out_put_cpu_clk; + }
On 24 July 2014 00:47, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: > It's this: > > +static int tegra124_cpufreq_probe(struct platform_device *pdev) > +{ > [...] > + > + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll"); > + if (IS_ERR(dfll_clk)) { > + ret = PTR_ERR(dfll_clk); > + goto out_put_cpu_clk; > + } This would search for clocks passed via DT, right? Why would we get EPROBE_DEFER for that? Sorry for the stupid question. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 24, 2014 at 05:43:40AM +0530, Viresh Kumar wrote: > On 24 July 2014 00:47, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote: > > It's this: > > > > +static int tegra124_cpufreq_probe(struct platform_device *pdev) > > +{ > > [...] > > + > > + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll"); > > + if (IS_ERR(dfll_clk)) { > > + ret = PTR_ERR(dfll_clk); > > + goto out_put_cpu_clk; > > + } > > This would search for clocks passed via DT, right? Why would we > get EPROBE_DEFER for that? Sorry for the stupid question. of_clk_get_by_name() can return -EPROBE_DEFER, which will happen if the clock provider hasn't been registered yet. Thierry
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 7364a53..df3c73e 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ config ARM_TEGRA_CPUFREQ bool "TEGRA CPUFreq support" depends on ARCH_TEGRA + depends on GENERIC_CPUFREQ_CPU0 default y help This adds the CPUFreq driver support for TEGRA SOCs. diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index db6d9a2..3437d24 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -76,6 +76,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_TEGRA_CPUFREQ) += tegra-cpufreq.o +obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra124-cpufreq.o obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o ################################################################################## diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c new file mode 100644 index 0000000..65ff428 --- /dev/null +++ b/drivers/cpufreq/tegra124-cpufreq.c @@ -0,0 +1,169 @@ +/* + * 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. + * + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/clk.h> +#include <linux/cpufreq.h> +#include <linux/cpu.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_opp.h> +#include <linux/types.h> + +static struct cpufreq_frequency_table *freq_table; + +static struct device *cpu_dev; +static struct clk *cpu_clk; +static struct clk *pllp_clk; +static struct clk *pllx_clk; +static struct clk *dfll_clk; + +static int tegra124_cpu_switch_to_dfll(void) +{ + struct clk *original_cpu_clk_parent; + unsigned long rate; + struct dev_pm_opp *opp; + int ret; + + rate = clk_get_rate(cpu_clk); + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); + if (IS_ERR(opp)) + return PTR_ERR(opp); + + ret = clk_set_rate(dfll_clk, rate); + if (ret) + return ret; + + original_cpu_clk_parent = clk_get_parent(cpu_clk); + clk_set_parent(cpu_clk, pllp_clk); + if (ret) + return ret; + + ret = clk_prepare_enable(dfll_clk); + if (ret) + goto out_switch_to_original_parent; + + clk_set_parent(cpu_clk, dfll_clk); + + return 0; + +out_switch_to_original_parent: + clk_set_parent(cpu_clk, original_cpu_clk_parent); + + return ret; +} + +static struct platform_device_info cpufreq_cpu0_devinfo = { + .name = "cpufreq-cpu0", +}; + +static int tegra124_cpufreq_probe(struct platform_device *pdev) +{ + int ret; + + cpu_dev = get_cpu_device(0); + if (!cpu_dev) + return -ENODEV; + + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g"); + if (IS_ERR(cpu_clk)) + return PTR_ERR(cpu_clk); + + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll"); + if (IS_ERR(dfll_clk)) { + ret = PTR_ERR(dfll_clk); + goto out_put_cpu_clk; + } + + pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x"); + if (IS_ERR(pllx_clk)) { + ret = PTR_ERR(pllx_clk); + goto out_put_dfll_clk; + } + + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p"); + if (IS_ERR(pllp_clk)) { + ret = PTR_ERR(pllp_clk); + goto out_put_pllx_clk; + } + + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); + if (ret) + goto out_put_pllp_clk; + + ret = tegra124_cpu_switch_to_dfll(); + if (ret) + goto out_free_table; + + platform_device_register_full(&cpufreq_cpu0_devinfo); + + return 0; + +out_free_table: + dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); +out_put_pllp_clk: + clk_put(pllp_clk); +out_put_pllx_clk: + clk_put(pllx_clk); +out_put_dfll_clk: + clk_put(dfll_clk); +out_put_cpu_clk: + clk_put(cpu_clk); + + return ret; +} + +static struct platform_driver tegra124_cpufreq_platdrv = { + .driver = { + .name = "cpufreq-tegra124", + .owner = THIS_MODULE, + }, + .probe = tegra124_cpufreq_probe, +}; + +static const struct of_device_id soc_of_matches[] = { + { .compatible = "nvidia,tegra124", }, + {} +}; + +static int __init tegra_cpufreq_init(void) +{ + int ret; + struct platform_device *pdev; + + if (!of_find_matching_node(NULL, soc_of_matches)) + return -ENODEV; + + ret = platform_driver_register(&tegra124_cpufreq_platdrv); + if (ret) + return ret; + + pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); + if (IS_ERR(pdev)) { + platform_driver_unregister(&tegra124_cpufreq_platdrv); + return PTR_ERR(pdev); + } + + return 0; +} + +MODULE_AUTHOR("Tuomas Tynkkynen <ttynkkynen@nvidia.com>"); +MODULE_DESCRIPTION("cpufreq driver for nVIDIA Tegra124"); +MODULE_LICENSE("GPLv2"); +module_init(tegra_cpufreq_init);
Add a new cpufreq driver for Tegra124. Instead of using the PLLX as the CPU clocksource, switch immediately to the DFLL. It allows the use of higher clock rates, and will automatically scale the CPU voltage as well. Besides the CPU clocksource switch, we let the cpufreq-cpu0 driver for all the cpufreq operations. This driver also relies on the DFLL driver to fill the OPP table for the CPU0 device, so that the cpufreq-cpu0 driver knows what frequencies to use. This driver is a completely independent of the old cpufreq driver (tegra-cpufreq), which is only used on Tegra20. Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com> --- v2 changes: - use the cpufreq-cpu0 driver drivers/cpufreq/Kconfig.arm | 1 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/tegra124-cpufreq.c | 169 +++++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+) create mode 100644 drivers/cpufreq/tegra124-cpufreq.c