| Message ID | 20250508-tegra124-cpufreq-v4-2-d142bcbd0234@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] cpufreq: tegra124: Remove use of disable_cpufreq | expand |
On 09/05/2025 01:04, Aaron Kling via B4 Relay wrote: > From: Aaron Kling <webgeek1234@gmail.com> > > This requires three changes: > * A soft dependency on cpufreq-dt as this driver only handles power > management and cpufreq-dt does the real operations Hmmm .. how is this handled for other drivers using the cpufreq-dt driver? I see the imx driver has a dependency on this. > * Adding a remove routine to remove the cpufreq-dt device > * Adding a exit routine to handle cleaning up the driver > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com> > --- > drivers/cpufreq/Kconfig.arm | 2 +- > drivers/cpufreq/tegra124-cpufreq.c | 36 ++++++++++++++++++++++++++++++++---- > 2 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index 4f9cb943d945c244eb2b29f543d14df6cac4e5d4..625f6fbdaaf5fd774e3b0bb996eb7ce980da41ee 100644 > --- a/drivers/cpufreq/Kconfig.arm > +++ b/drivers/cpufreq/Kconfig.arm > @@ -238,7 +238,7 @@ config ARM_TEGRA20_CPUFREQ > This adds the CPUFreq driver support for Tegra20/30 SOCs. > > config ARM_TEGRA124_CPUFREQ > - bool "Tegra124 CPUFreq support" > + tristate "Tegra124 CPUFreq support" > depends on ARCH_TEGRA || COMPILE_TEST > depends on CPUFREQ_DT > default y > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c > index bc0691e8971f9454def37f489e4a3e244100b9f4..b6059c91f2474c56809c403eca94eacf51df734f 100644 > --- a/drivers/cpufreq/tegra124-cpufreq.c > +++ b/drivers/cpufreq/tegra124-cpufreq.c > @@ -16,6 +16,8 @@ > #include <linux/pm_opp.h> > #include <linux/types.h> > > +static struct platform_device *platform_device; Do we need this? > + > struct tegra124_cpufreq_priv { > struct clk *cpu_clk; > struct clk *pllp_clk; > @@ -176,6 +178,21 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev) > return err; > } > > +static void tegra124_cpufreq_remove(struct platform_device *pdev) > +{ > + struct tegra124_cpufreq_priv *priv = dev_get_drvdata(&pdev->dev); > + > + if (!IS_ERR(priv->cpufreq_dt_pdev)) { > + platform_device_unregister(priv->cpufreq_dt_pdev); > + priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV); > + } > + > + clk_put(priv->pllp_clk); > + clk_put(priv->pllx_clk); > + clk_put(priv->dfll_clk); > + clk_put(priv->cpu_clk); > +} > + > static const struct dev_pm_ops tegra124_cpufreq_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(tegra124_cpufreq_suspend, > tegra124_cpufreq_resume) > @@ -185,12 +202,12 @@ static struct platform_driver tegra124_cpufreq_platdrv = { > .driver.name = "cpufreq-tegra124", > .driver.pm = &tegra124_cpufreq_pm_ops, > .probe = tegra124_cpufreq_probe, > + .remove = tegra124_cpufreq_remove, > }; > > static int __init tegra_cpufreq_init(void) > { > int ret; > - struct platform_device *pdev; > > if (!(of_machine_is_compatible("nvidia,tegra124") || > of_machine_is_compatible("nvidia,tegra210"))) > @@ -204,15 +221,26 @@ static int __init tegra_cpufreq_init(void) > if (ret) > return ret; > > - pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); > - if (IS_ERR(pdev)) { > + platform_device = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); > + if (IS_ERR(platform_device)) { > platform_driver_unregister(&tegra124_cpufreq_platdrv); > - return PTR_ERR(pdev); > + return PTR_ERR(platform_device); > } > > return 0; > } > module_init(tegra_cpufreq_init); > > +static void __exit tegra_cpufreq_module_exit(void) > +{ > + if (platform_device && !IS_ERR(platform_device)) > + platform_device_unregister(platform_device); The device is unregistered in the remove. Why do we need this? > + > + platform_driver_unregister(&tegra124_cpufreq_platdrv); > +} > +module_exit(tegra_cpufreq_module_exit); > + > +MODULE_SOFTDEP("pre: cpufreq-dt"); > MODULE_AUTHOR("Tuomas Tynkkynen <ttynkkynen@nvidia.com>"); > MODULE_DESCRIPTION("cpufreq driver for NVIDIA Tegra124"); > +MODULE_LICENSE("GPL"); >
On Fri, May 9, 2025 at 8:38 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > On 09/05/2025 01:04, Aaron Kling via B4 Relay wrote: > > From: Aaron Kling <webgeek1234@gmail.com> > > > > This requires three changes: > > * A soft dependency on cpufreq-dt as this driver only handles power > > management and cpufreq-dt does the real operations > > Hmmm .. how is this handled for other drivers using the cpufreq-dt > driver? I see the imx driver has a dependency on this. A hard dependency would likely make more sense here. I can update this in a new revision. When I first set the soft dependency, I wasn't certain how the driver worked, so I was trying to be less intrusive. > > * Adding a remove routine to remove the cpufreq-dt device > > * Adding a exit routine to handle cleaning up the driver > > > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com> > > --- > > drivers/cpufreq/Kconfig.arm | 2 +- > > drivers/cpufreq/tegra124-cpufreq.c | 36 ++++++++++++++++++++++++++++++++---- > > 2 files changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > > index 4f9cb943d945c244eb2b29f543d14df6cac4e5d4..625f6fbdaaf5fd774e3b0bb996eb7ce980da41ee 100644 > > --- a/drivers/cpufreq/Kconfig.arm > > +++ b/drivers/cpufreq/Kconfig.arm > > @@ -238,7 +238,7 @@ config ARM_TEGRA20_CPUFREQ > > This adds the CPUFreq driver support for Tegra20/30 SOCs. > > > > config ARM_TEGRA124_CPUFREQ > > - bool "Tegra124 CPUFreq support" > > + tristate "Tegra124 CPUFreq support" > > depends on ARCH_TEGRA || COMPILE_TEST > > depends on CPUFREQ_DT > > default y > > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c > > index bc0691e8971f9454def37f489e4a3e244100b9f4..b6059c91f2474c56809c403eca94eacf51df734f 100644 > > --- a/drivers/cpufreq/tegra124-cpufreq.c > > +++ b/drivers/cpufreq/tegra124-cpufreq.c > > @@ -16,6 +16,8 @@ > > #include <linux/pm_opp.h> > > #include <linux/types.h> > > > > +static struct platform_device *platform_device; > > Do we need this? > > > + > > struct tegra124_cpufreq_priv { > > struct clk *cpu_clk; > > struct clk *pllp_clk; > > @@ -176,6 +178,21 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev) > > return err; > > } > > > > +static void tegra124_cpufreq_remove(struct platform_device *pdev) > > +{ > > + struct tegra124_cpufreq_priv *priv = dev_get_drvdata(&pdev->dev); > > + > > + if (!IS_ERR(priv->cpufreq_dt_pdev)) { > > + platform_device_unregister(priv->cpufreq_dt_pdev); > > + priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV); > > + } > > + > > + clk_put(priv->pllp_clk); > > + clk_put(priv->pllx_clk); > > + clk_put(priv->dfll_clk); > > + clk_put(priv->cpu_clk); > > +} > > + > > static const struct dev_pm_ops tegra124_cpufreq_pm_ops = { > > SET_SYSTEM_SLEEP_PM_OPS(tegra124_cpufreq_suspend, > > tegra124_cpufreq_resume) > > @@ -185,12 +202,12 @@ static struct platform_driver tegra124_cpufreq_platdrv = { > > .driver.name = "cpufreq-tegra124", > > .driver.pm = &tegra124_cpufreq_pm_ops, > > .probe = tegra124_cpufreq_probe, > > + .remove = tegra124_cpufreq_remove, > > }; > > > > static int __init tegra_cpufreq_init(void) > > { > > int ret; > > - struct platform_device *pdev; > > > > if (!(of_machine_is_compatible("nvidia,tegra124") || > > of_machine_is_compatible("nvidia,tegra210"))) > > @@ -204,15 +221,26 @@ static int __init tegra_cpufreq_init(void) > > if (ret) > > return ret; > > > > - pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); > > - if (IS_ERR(pdev)) { > > + platform_device = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); > > + if (IS_ERR(platform_device)) { > > platform_driver_unregister(&tegra124_cpufreq_platdrv); > > - return PTR_ERR(pdev); > > + return PTR_ERR(platform_device); > > } > > > > return 0; > > } > > module_init(tegra_cpufreq_init); > > > > +static void __exit tegra_cpufreq_module_exit(void) > > +{ > > + if (platform_device && !IS_ERR(platform_device)) > > + platform_device_unregister(platform_device); > > The device is unregistered in the remove. Why do we need this? These are separate things, aren't they? What's unregistered in the remove is the cpufreq-dt device. And what's unregistered here is the tegra124-cpufreq device. Not the same thing, unless I'm really missing something. Sincerely, Aaron
On 09/05/2025 01:04, Aaron Kling via B4 Relay wrote: > From: Aaron Kling <webgeek1234@gmail.com> > > This requires three changes: > * A soft dependency on cpufreq-dt as this driver only handles power > management and cpufreq-dt does the real operations > * Adding a remove routine to remove the cpufreq-dt device > * Adding a exit routine to handle cleaning up the driver > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com> > --- > drivers/cpufreq/Kconfig.arm | 2 +- > drivers/cpufreq/tegra124-cpufreq.c | 36 ++++++++++++++++++++++++++++++++---- > 2 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index 4f9cb943d945c244eb2b29f543d14df6cac4e5d4..625f6fbdaaf5fd774e3b0bb996eb7ce980da41ee 100644 > --- a/drivers/cpufreq/Kconfig.arm > +++ b/drivers/cpufreq/Kconfig.arm > @@ -238,7 +238,7 @@ config ARM_TEGRA20_CPUFREQ > This adds the CPUFreq driver support for Tegra20/30 SOCs. > > config ARM_TEGRA124_CPUFREQ > - bool "Tegra124 CPUFreq support" > + tristate "Tegra124 CPUFreq support" > depends on ARCH_TEGRA || COMPILE_TEST > depends on CPUFREQ_DT > default y > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c > index bc0691e8971f9454def37f489e4a3e244100b9f4..b6059c91f2474c56809c403eca94eacf51df734f 100644 > --- a/drivers/cpufreq/tegra124-cpufreq.c > +++ b/drivers/cpufreq/tegra124-cpufreq.c > @@ -16,6 +16,8 @@ > #include <linux/pm_opp.h> > #include <linux/types.h> > > +static struct platform_device *platform_device; > + > struct tegra124_cpufreq_priv { > struct clk *cpu_clk; > struct clk *pllp_clk; > @@ -176,6 +178,21 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev) > return err; > } > > +static void tegra124_cpufreq_remove(struct platform_device *pdev) > +{ > + struct tegra124_cpufreq_priv *priv = dev_get_drvdata(&pdev->dev); > + > + if (!IS_ERR(priv->cpufreq_dt_pdev)) { > + platform_device_unregister(priv->cpufreq_dt_pdev); > + priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV); > + } > + > + clk_put(priv->pllp_clk); > + clk_put(priv->pllx_clk); > + clk_put(priv->dfll_clk); > + clk_put(priv->cpu_clk); If we use devm_clk_get() in probe, then we should be able to avoid this. Jon
On Wed, May 14, 2025 at 5:32 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 09/05/2025 01:04, Aaron Kling via B4 Relay wrote: > > From: Aaron Kling <webgeek1234@gmail.com> > > > > This requires three changes: > > * A soft dependency on cpufreq-dt as this driver only handles power > > management and cpufreq-dt does the real operations > > * Adding a remove routine to remove the cpufreq-dt device > > * Adding a exit routine to handle cleaning up the driver > > > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com> > > --- > > drivers/cpufreq/Kconfig.arm | 2 +- > > drivers/cpufreq/tegra124-cpufreq.c | 36 ++++++++++++++++++++++++++++++++---- > > 2 files changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > > index 4f9cb943d945c244eb2b29f543d14df6cac4e5d4..625f6fbdaaf5fd774e3b0bb996eb7ce980da41ee 100644 > > --- a/drivers/cpufreq/Kconfig.arm > > +++ b/drivers/cpufreq/Kconfig.arm > > @@ -238,7 +238,7 @@ config ARM_TEGRA20_CPUFREQ > > This adds the CPUFreq driver support for Tegra20/30 SOCs. > > > > config ARM_TEGRA124_CPUFREQ > > - bool "Tegra124 CPUFreq support" > > + tristate "Tegra124 CPUFreq support" > > depends on ARCH_TEGRA || COMPILE_TEST > > depends on CPUFREQ_DT > > default y > > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c > > index bc0691e8971f9454def37f489e4a3e244100b9f4..b6059c91f2474c56809c403eca94eacf51df734f 100644 > > --- a/drivers/cpufreq/tegra124-cpufreq.c > > +++ b/drivers/cpufreq/tegra124-cpufreq.c > > @@ -16,6 +16,8 @@ > > #include <linux/pm_opp.h> > > #include <linux/types.h> > > > > +static struct platform_device *platform_device; > > + > > struct tegra124_cpufreq_priv { > > struct clk *cpu_clk; > > struct clk *pllp_clk; > > @@ -176,6 +178,21 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev) > > return err; > > } > > > > +static void tegra124_cpufreq_remove(struct platform_device *pdev) > > +{ > > + struct tegra124_cpufreq_priv *priv = dev_get_drvdata(&pdev->dev); > > + > > + if (!IS_ERR(priv->cpufreq_dt_pdev)) { > > + platform_device_unregister(priv->cpufreq_dt_pdev); > > + priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV); > > + } > > + > > + clk_put(priv->pllp_clk); > > + clk_put(priv->pllx_clk); > > + clk_put(priv->dfll_clk); > > + clk_put(priv->cpu_clk); > > > If we use devm_clk_get() in probe, then we should be able to avoid this. I can do that. There's a lot of other cleanup like this that the driver could use based on newer kernel apis, but that's out of scope of this series. Sincerely, Aaron
On Mon, May 12, 2025 at 11:26 PM Aaron Kling <webgeek1234@gmail.com> wrote: > > On Fri, May 9, 2025 at 8:38 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > > > > > On 09/05/2025 01:04, Aaron Kling via B4 Relay wrote: > > > From: Aaron Kling <webgeek1234@gmail.com> > > > > > > This requires three changes: > > > * A soft dependency on cpufreq-dt as this driver only handles power > > > management and cpufreq-dt does the real operations > > > > Hmmm .. how is this handled for other drivers using the cpufreq-dt > > driver? I see the imx driver has a dependency on this. > > A hard dependency would likely make more sense here. I can update this > in a new revision. When I first set the soft dependency, I wasn't > certain how the driver worked, so I was trying to be less intrusive. I remember why I added this soft dep now. The kconfig already has a dependency on cpufreq_dt. However, this driver doesn't call any functions directly in that driver. It just builds a platform device struct for it, then registers it. This results in depmod not requiring cpufreq_dt for tegra124_cpufreq. So I added the softdep to work around that, so modprobing tegra124_cpufreq by itself functions properly. Is there a better way to make depmod map this as needed? Sincerely, Aaron
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 4f9cb943d945c244eb2b29f543d14df6cac4e5d4..625f6fbdaaf5fd774e3b0bb996eb7ce980da41ee 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -238,7 +238,7 @@ config ARM_TEGRA20_CPUFREQ This adds the CPUFreq driver support for Tegra20/30 SOCs. config ARM_TEGRA124_CPUFREQ - bool "Tegra124 CPUFreq support" + tristate "Tegra124 CPUFreq support" depends on ARCH_TEGRA || COMPILE_TEST depends on CPUFREQ_DT default y diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c index bc0691e8971f9454def37f489e4a3e244100b9f4..b6059c91f2474c56809c403eca94eacf51df734f 100644 --- a/drivers/cpufreq/tegra124-cpufreq.c +++ b/drivers/cpufreq/tegra124-cpufreq.c @@ -16,6 +16,8 @@ #include <linux/pm_opp.h> #include <linux/types.h> +static struct platform_device *platform_device; + struct tegra124_cpufreq_priv { struct clk *cpu_clk; struct clk *pllp_clk; @@ -176,6 +178,21 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev) return err; } +static void tegra124_cpufreq_remove(struct platform_device *pdev) +{ + struct tegra124_cpufreq_priv *priv = dev_get_drvdata(&pdev->dev); + + if (!IS_ERR(priv->cpufreq_dt_pdev)) { + platform_device_unregister(priv->cpufreq_dt_pdev); + priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV); + } + + clk_put(priv->pllp_clk); + clk_put(priv->pllx_clk); + clk_put(priv->dfll_clk); + clk_put(priv->cpu_clk); +} + static const struct dev_pm_ops tegra124_cpufreq_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(tegra124_cpufreq_suspend, tegra124_cpufreq_resume) @@ -185,12 +202,12 @@ static struct platform_driver tegra124_cpufreq_platdrv = { .driver.name = "cpufreq-tegra124", .driver.pm = &tegra124_cpufreq_pm_ops, .probe = tegra124_cpufreq_probe, + .remove = tegra124_cpufreq_remove, }; static int __init tegra_cpufreq_init(void) { int ret; - struct platform_device *pdev; if (!(of_machine_is_compatible("nvidia,tegra124") || of_machine_is_compatible("nvidia,tegra210"))) @@ -204,15 +221,26 @@ static int __init tegra_cpufreq_init(void) if (ret) return ret; - pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); - if (IS_ERR(pdev)) { + platform_device = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0); + if (IS_ERR(platform_device)) { platform_driver_unregister(&tegra124_cpufreq_platdrv); - return PTR_ERR(pdev); + return PTR_ERR(platform_device); } return 0; } module_init(tegra_cpufreq_init); +static void __exit tegra_cpufreq_module_exit(void) +{ + if (platform_device && !IS_ERR(platform_device)) + platform_device_unregister(platform_device); + + platform_driver_unregister(&tegra124_cpufreq_platdrv); +} +module_exit(tegra_cpufreq_module_exit); + +MODULE_SOFTDEP("pre: cpufreq-dt"); MODULE_AUTHOR("Tuomas Tynkkynen <ttynkkynen@nvidia.com>"); MODULE_DESCRIPTION("cpufreq driver for NVIDIA Tegra124"); +MODULE_LICENSE("GPL");