[PATCH v4 0/2] Support building tegra124-cpufreq as a module

[PATCH v4 0/2] Support building tegra124-cpufreq as a module

[Aaron Kling via B4 Relay]

This adds remove and exit routines that were not previously needed when
this was only available builtin. It also converts use of an unexported
function to a more sane alternative.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
Changes in v4:
- Move clock puts to remove instead of exit
- Link to v3: https://lore.kernel.org/r/20250421-tegra124-cpufreq-v3-0-95eaba968467@gmail.com

Changes in v3:
- In patch 1, set cpufreq_dt_pdev to an error after unregister on fail
  to prevent a potential double unregister on remove
- In patch 2, clean up clocks on exit
- Link to v2: https://lore.kernel.org/r/20250421-tegra124-cpufreq-v2-0-2f148cefa418@gmail.com

Changes in v2:
- Replace patch 1 with a patch to not use the unexported function
- Update patch 2 to add remove and exit routines
- Link to v1: https://lore.kernel.org/r/20250420-tegra124-cpufreq-v1-0-0a47fe126091@gmail.com

---
Aaron Kling (2):
      cpufreq: tegra124: Remove use of disable_cpufreq
      cpufreq: tegra124: Allow building as a module

 drivers/cpufreq/Kconfig.arm        |  2 +-
 drivers/cpufreq/tegra124-cpufreq.c | 41 +++++++++++++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 6 deletions(-)
---
base-commit: 91e5bfe317d8f8471fbaa3e70cf66cae1314a516
change-id: 20250401-tegra124-cpufreq-fd944fdce62c

Best regards,
-- 
Aaron Kling <webgeek1234@gmail.com>

[PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Aaron Kling via B4 Relay]

From: Aaron Kling <webgeek1234@gmail.com>

Instead, unregister the cpufreq device for this fatal fail case.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
 drivers/cpufreq/tegra124-cpufreq.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
index 514146d98bca2d8aa59980a14dff3487cd8045f6..bc0691e8971f9454def37f489e4a3e244100b9f4 100644
--- a/drivers/cpufreq/tegra124-cpufreq.c
+++ b/drivers/cpufreq/tegra124-cpufreq.c
@@ -168,7 +168,10 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev)
 disable_dfll:
 	clk_disable_unprepare(priv->dfll_clk);
 disable_cpufreq:
-	disable_cpufreq();
+	if (!IS_ERR(priv->cpufreq_dt_pdev)) {
+		platform_device_unregister(priv->cpufreq_dt_pdev);
+		priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV);
+	}
 
 	return err;
 }

-- 
2.48.1

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Jon Hunter]

On 09/05/2025 01:04, Aaron Kling via B4 Relay wrote:
> From: Aaron Kling <webgeek1234@gmail.com>
> 
> Instead, unregister the cpufreq device for this fatal fail case.

This is not a complete sentence. Seems to be a continuation of the 
subject which is not clear to the reader (at least not to me). No 
mention of why or what this is fixing, if anything?

> 
> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> ---
>   drivers/cpufreq/tegra124-cpufreq.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> index 514146d98bca2d8aa59980a14dff3487cd8045f6..bc0691e8971f9454def37f489e4a3e244100b9f4 100644
> --- a/drivers/cpufreq/tegra124-cpufreq.c
> +++ b/drivers/cpufreq/tegra124-cpufreq.c
> @@ -168,7 +168,10 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev)
>   disable_dfll:
>   	clk_disable_unprepare(priv->dfll_clk);
>   disable_cpufreq:
> -	disable_cpufreq();
> +	if (!IS_ERR(priv->cpufreq_dt_pdev)) {
> +		platform_device_unregister(priv->cpufreq_dt_pdev);
> +		priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV);
> +	}

So you are proposing to unregister the device in resume? That seems odd. 
I see there is no remove for this driver, but I really don't see the 
value in this.

Jon

-- 
nvpublic

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Aaron Kling]

On Fri, May 9, 2025 at 6:04 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>
> >
> > Instead, unregister the cpufreq device for this fatal fail case.
>
> This is not a complete sentence. Seems to be a continuation of the
> subject which is not clear to the reader (at least not to me). No
> mention of why or what this is fixing, if anything?

I can clean up the subject and message in a new revision. More on the
reasoning below.

> >
> > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > ---
> >   drivers/cpufreq/tegra124-cpufreq.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> > index 514146d98bca2d8aa59980a14dff3487cd8045f6..bc0691e8971f9454def37f489e4a3e244100b9f4 100644
> > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > @@ -168,7 +168,10 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev)
> >   disable_dfll:
> >       clk_disable_unprepare(priv->dfll_clk);
> >   disable_cpufreq:
> > -     disable_cpufreq();
> > +     if (!IS_ERR(priv->cpufreq_dt_pdev)) {
> > +             platform_device_unregister(priv->cpufreq_dt_pdev);
> > +             priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV);
> > +     }
>
> So you are proposing to unregister the device in resume? That seems odd.
> I see there is no remove for this driver, but I really don't see the
> value in this.

This was suggested by Viresh in v1 [0] to replace the call to
disable_cpufreq, which is not currently an exported function. I'm open
to other suggestions.

Sincerely,
Aaron

[0] https://lore.kernel.org/all/20250421054452.fdlrrhtsimyucbup@vireshk-i7/

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Aaron Kling]

On Fri, May 9, 2025 at 11:57 AM Aaron Kling <webgeek1234@gmail.com> wrote:
>
> On Fri, May 9, 2025 at 6:04 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>
> > >
> > > Instead, unregister the cpufreq device for this fatal fail case.
> >
> > This is not a complete sentence. Seems to be a continuation of the
> > subject which is not clear to the reader (at least not to me). No
> > mention of why or what this is fixing, if anything?
>
> I can clean up the subject and message in a new revision. More on the
> reasoning below.
>
> > >
> > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > > ---
> > >   drivers/cpufreq/tegra124-cpufreq.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> > > index 514146d98bca2d8aa59980a14dff3487cd8045f6..bc0691e8971f9454def37f489e4a3e244100b9f4 100644
> > > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > > @@ -168,7 +168,10 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev)
> > >   disable_dfll:
> > >       clk_disable_unprepare(priv->dfll_clk);
> > >   disable_cpufreq:
> > > -     disable_cpufreq();
> > > +     if (!IS_ERR(priv->cpufreq_dt_pdev)) {
> > > +             platform_device_unregister(priv->cpufreq_dt_pdev);
> > > +             priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV);
> > > +     }
> >
> > So you are proposing to unregister the device in resume? That seems odd.
> > I see there is no remove for this driver, but I really don't see the
> > value in this.
>
> This was suggested by Viresh in v1 [0] to replace the call to
> disable_cpufreq, which is not currently an exported function. I'm open
> to other suggestions.
>
> Sincerely,
> Aaron
>
> [0] https://lore.kernel.org/all/20250421054452.fdlrrhtsimyucbup@vireshk-i7/

Viresh, could you comment here please? As you were the one that
suggested replacing disable_cpufreq with an unregister instead of
exporting said function. I can make the code changes and verify they
work as intended, but I'm not familiar enough with this subsystem to
know what a good option here is. Are there any other cpufreq drivers
that have to handle a resume failure like this?

Sincerely,
Aaron

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Viresh Kumar]

On 09-05-25, 12:04, Jon Hunter wrote:
> > diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
> > index 514146d98bca2d8aa59980a14dff3487cd8045f6..bc0691e8971f9454def37f489e4a3e244100b9f4 100644
> > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > @@ -168,7 +168,10 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev)
> >   disable_dfll:
> >   	clk_disable_unprepare(priv->dfll_clk);
> >   disable_cpufreq:
> > -	disable_cpufreq();
> > +	if (!IS_ERR(priv->cpufreq_dt_pdev)) {
> > +		platform_device_unregister(priv->cpufreq_dt_pdev);
> > +		priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV);
> > +	}
> 
> So you are proposing to unregister the device in resume? That seems odd. I
> see there is no remove for this driver, but I really don't see the value in
> this.

This is the failure path and the driver is trying to disable itself
here. Instead of using the disable_cpufreq() (which isn't designed for
this usecase), I suggested removing the device itself as the driver
will be unusable after this anyway.

-- 
viresh

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Jon Hunter]

On 19/05/2025 11:17, Viresh Kumar wrote:
> On 09-05-25, 12:04, Jon Hunter wrote:
>>> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
>>> index 514146d98bca2d8aa59980a14dff3487cd8045f6..bc0691e8971f9454def37f489e4a3e244100b9f4 100644
>>> --- a/drivers/cpufreq/tegra124-cpufreq.c
>>> +++ b/drivers/cpufreq/tegra124-cpufreq.c
>>> @@ -168,7 +168,10 @@ static int __maybe_unused tegra124_cpufreq_resume(struct device *dev)
>>>    disable_dfll:
>>>    	clk_disable_unprepare(priv->dfll_clk);
>>>    disable_cpufreq:
>>> -	disable_cpufreq();
>>> +	if (!IS_ERR(priv->cpufreq_dt_pdev)) {
>>> +		platform_device_unregister(priv->cpufreq_dt_pdev);
>>> +		priv->cpufreq_dt_pdev = ERR_PTR(-ENODEV);
>>> +	}
>>
>> So you are proposing to unregister the device in resume? That seems odd. I
>> see there is no remove for this driver, but I really don't see the value in
>> this.
> 
> This is the failure path and the driver is trying to disable itself
> here. Instead of using the disable_cpufreq() (which isn't designed for
> this usecase), I suggested removing the device itself as the driver
> will be unusable after this anyway.


I understand, but this seems odd. It would be odd that the device may 
just disappear after resuming from suspend if it fails to resume. I have 
not seen this done for other drivers that fail to resume. Presumably 
this is not the only CPU Freq driver that could fail to resume either?

It makes the code messy because now we have more than one place where 
the device could be unregistered.

Jon

-- 
nvpublic

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Viresh Kumar]

On 20-05-25, 10:53, Jon Hunter wrote:
> I understand, but this seems odd. It would be odd that the device may just
> disappear after resuming from suspend if it fails to resume. I have not seen
> this done for other drivers that fail to resume. Presumably this is not the
> only CPU Freq driver that could fail to resume either?
> 
> It makes the code messy because now we have more than one place where the
> device could be unregistered.

Fair enough.

This driver, along with other cpufreq drivers, can fail at multiple
places during suspend/resume (and other operations). If something goes
wrong, we print an error to inform the user. Should we avoid doing
anything else (like everyone else) ? i.e. Just remove the call to
disable_cpufreq(), as all later calls will fail anyway.

This is the only driver that behaves differently on failures.

-- 
viresh

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Aaron Kling]

On Tue, May 20, 2025 at 5:02 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 20-05-25, 10:53, Jon Hunter wrote:
> > I understand, but this seems odd. It would be odd that the device may just
> > disappear after resuming from suspend if it fails to resume. I have not seen
> > this done for other drivers that fail to resume. Presumably this is not the
> > only CPU Freq driver that could fail to resume either?
> >
> > It makes the code messy because now we have more than one place where the
> > device could be unregistered.
>
> Fair enough.
>
> This driver, along with other cpufreq drivers, can fail at multiple
> places during suspend/resume (and other operations). If something goes
> wrong, we print an error to inform the user. Should we avoid doing
> anything else (like everyone else) ? i.e. Just remove the call to
> disable_cpufreq(), as all later calls will fail anyway.
>
> This is the only driver that behaves differently on failures.
>
> --
> viresh

Is there any consensus on the best way to handle this? I'd like to
keep the series moving.

Sincerely,
Aaron

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Viresh Kumar]

On 28-05-25, 12:29, Aaron Kling wrote:
> Is there any consensus on the best way to handle this? I'd like to
> keep the series moving.

I was waiting for Jon to provide further feedback here.

-- 
viresh

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Jon Hunter]

On 20/05/2025 11:02, Viresh Kumar wrote:
> On 20-05-25, 10:53, Jon Hunter wrote:
>> I understand, but this seems odd. It would be odd that the device may just
>> disappear after resuming from suspend if it fails to resume. I have not seen
>> this done for other drivers that fail to resume. Presumably this is not the
>> only CPU Freq driver that could fail to resume either?
>>
>> It makes the code messy because now we have more than one place where the
>> device could be unregistered.
> 
> Fair enough.
> 
> This driver, along with other cpufreq drivers, can fail at multiple
> places during suspend/resume (and other operations). If something goes
> wrong, we print an error to inform the user. Should we avoid doing
> anything else (like everyone else) ? i.e. Just remove the call to
> disable_cpufreq(), as all later calls will fail anyway.

I think that would be fine. Given that the tegra124-cpufreq driver is 
the parent, if it fails to resume, then I assume that cpufreq-dt driver 
would not resume either? Has anyone tested this?

Jon

-- 
nvpublic

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Viresh Kumar]

On 05-06-25, 11:34, Jon Hunter wrote:
> I think that would be fine. Given that the tegra124-cpufreq driver is the
> parent, if it fails to resume, then I assume that cpufreq-dt driver would
> not resume either?

There is no resume interface in the cpufreq-dt driver, it is the cpufreq core
which resumes to doing DVFS and I think it will try to do DVFS even if tegra's
driver failed.

> Has anyone tested this?

-- 
viresh

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Aaron Kling]

On Thu, Jun 5, 2025 at 5:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 05-06-25, 11:34, Jon Hunter wrote:
> > I think that would be fine. Given that the tegra124-cpufreq driver is the
> > parent, if it fails to resume, then I assume that cpufreq-dt driver would
> > not resume either?
>
> There is no resume interface in the cpufreq-dt driver, it is the cpufreq core
> which resumes to doing DVFS and I think it will try to do DVFS even if tegra's
> driver failed.

In my opinion, I'm thinking the original flow makes more sense. If
resume fails, disable cpufreq. Then the subsystem doesn't keep trying
and failing and causing who knows what kind of havoc. But if that's
still not desired, what should I do to get this moving again? Just
drop the error handling entirely, as suggested?

Aaron

Re: [PATCH v4 1/2] cpufreq: tegra124: Remove use of disable_cpufreq

[Viresh Kumar]

On 30-06-25, 13:43, Aaron Kling wrote:
> In my opinion, I'm thinking the original flow makes more sense. If
> resume fails, disable cpufreq. Then the subsystem doesn't keep trying
> and failing and causing who knows what kind of havoc.

Lets do this and move ahead since we aren't able to conclude for a while.

-- 
viresh

[PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Aaron Kling via B4 Relay]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Jon Hunter]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Aaron Kling]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Jon Hunter]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Aaron Kling]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Jon Hunter]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Viresh Kumar]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Jon Hunter]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Viresh Kumar]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Aaron Kling]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Jon Hunter]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Aaron Kling]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Jon Hunter]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Viresh Kumar]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Jon Hunter]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Viresh Kumar]

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

Re: [PATCH v4 2/2] cpufreq: tegra124: Allow building as a module

[Jon Hunter]

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