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);
+}
+
 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");

-- 
2.48.1

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

-- 
nvpublic

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.

-- 
viresh

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

-- 
nvpublic

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.

-- 
viresh

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

-- 
nvpublic

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 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

-- 
nvpublic

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");
> 

-- 
nvpublic

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 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: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

-- 
nvpublic

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.

-- 
viresh

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

-- 
nvpublic

On 20-05-25, 10:57, Jon Hunter wrote:
> 
> 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.

Fair enough.

Aaron, you can introduce a helper like cpufreq_dt_pdev_register() to
solve the linking here.

-- 
viresh

On Tue, May 20, 2025 at 5:33 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 20-05-25, 10:57, Jon Hunter wrote:
> >
> > 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.
>
> Fair enough.
>
> Aaron, you can introduce a helper like cpufreq_dt_pdev_register() to
> solve the linking here.

Will do. I'll get it queued for when patch 1 is actionable.

Aaron

On 13/05/2025 05:26, Aaron Kling wrote:

...

>>> +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.


Thanks for the correction. OK, but I am still not sure about patch 1/2.

Jon

-- 
nvpublic