| 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
On 14/05/2025 17:30, Aaron Kling wrote: > 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. I don't think that using devm_clk_get() is out the scope here, because this would greatly simplify the remove. Jon
On 14/05/2025 17:43, Aaron Kling wrote: > 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? Yes and that is understood. I see a few drivers calling ... platform_device_register_simple("cpufreq-dt", -1, NULL, 0); One option, and I don't know if this would be acceptable, would be to add a new wrapper function in the cpufreq-dt driver for the above that other drivers could call and that would create the dependency you need. Jon
On 14-05-25, 11:31, Jon Hunter wrote: > > +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. Not sure if we can do that. The clks belong to the CPU device, while the devm_* functions are using &pdev->dev. The CPU device never goes away and so the resources won't get freed if we use devm for the CPU device.
On 15-05-25, 07:41, Jon Hunter wrote: > Yes and that is understood. I see a few drivers calling ... > > platform_device_register_simple("cpufreq-dt", -1, NULL, 0); > > One option, and I don't know if this would be acceptable, would be to add a > new wrapper function in the cpufreq-dt driver for the above that other > drivers could call and that would create the dependency you need. Doing that won't be a problem, but I doubt if that is a better than adding a soft dependency here. I personally felt that the soft dependency may be the right way here. The cpufreq-dt file presents a driver, a device can be added from any file and that doesn't require the driver file to be inserted first. If the platform wants to simplify and create a dependency, a soft dependency looks okay.
On 19/05/2025 11:37, Viresh Kumar wrote: > On 15-05-25, 07:41, Jon Hunter wrote: >> Yes and that is understood. I see a few drivers calling ... >> >> platform_device_register_simple("cpufreq-dt", -1, NULL, 0); >> >> One option, and I don't know if this would be acceptable, would be to add a >> new wrapper function in the cpufreq-dt driver for the above that other >> drivers could call and that would create the dependency you need. > > Doing that won't be a problem, but I doubt if that is a better than > adding a soft dependency here. I personally felt that the soft > dependency may be the right way here. The cpufreq-dt file presents a > driver, a device can be added from any file and that doesn't require > the driver file to be inserted first. If the platform wants to > simplify and create a dependency, a soft dependency looks okay. The only downside of a soft dependency is that this driver could load but if the cpufreq-dt driver is missing for whatever reason, it might not be obvious. Ideally it is better if this driver does not load at all if the cpufreq-dt is not present. Jon
On 19/05/2025 11:26, Viresh Kumar wrote: > On 14-05-25, 11:31, Jon Hunter wrote: >>> +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. > > Not sure if we can do that. The clks belong to the CPU device, while > the devm_* functions are using &pdev->dev. The CPU device never goes > away and so the resources won't get freed if we use devm for the CPU > device. I don't follow. If they are allocated in the probe using the pdev->dev device by using devm_clk_get() they should get freed when the platform device is removed. Jon
On 20-05-25, 11:03, Jon Hunter wrote: > On 19/05/2025 11:26, Viresh Kumar wrote: > > Not sure if we can do that. The clks belong to the CPU device, while > > the devm_* functions are using &pdev->dev. The CPU device never goes > > away and so the resources won't get freed if we use devm for the CPU > > device. That would have been the case, if we can actually do a devm_clk_get() in the first place, but... > I don't follow. If they are allocated in the probe using the pdev->dev > device by using devm_clk_get() they should get freed when the platform > device is removed. ... devm_clk_get(&pdev->dev, ...) won't work here IIUC. The clks belong to the CPU device and not pdev->dev. That's why we are doing of_clk_get_by_name() over the CPU device's OF node here. Maybe I am wrong, but I don't see how devm_* can be used here for clks.
On 20/05/2025 11:30, Viresh Kumar wrote: > On 20-05-25, 11:03, Jon Hunter wrote: >> On 19/05/2025 11:26, Viresh Kumar wrote: >>> Not sure if we can do that. The clks belong to the CPU device, while >>> the devm_* functions are using &pdev->dev. The CPU device never goes >>> away and so the resources won't get freed if we use devm for the CPU >>> device. > > That would have been the case, if we can actually do a devm_clk_get() > in the first place, but... > >> I don't follow. If they are allocated in the probe using the pdev->dev >> device by using devm_clk_get() they should get freed when the platform >> device is removed. > > ... devm_clk_get(&pdev->dev, ...) won't work here IIUC. The clks > belong to the CPU device and not pdev->dev. That's why we are doing > of_clk_get_by_name() over the CPU device's OF node here. > > Maybe I am wrong, but I don't see how devm_* can be used here for > clks. Ah yes, we are using the 'np' pointer for the CPU node and not the platform device node. OK scratch that. Jon
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");