From: Yi-Wei Wang <yiweiw@nvidia.com>
The clock driving the Tegra PWM IP can be sourced from different parent
clocks. Hence, let dev_pm_opp_set_rate() set the max clock rate based
upon the current parent clock that can be specified via device-tree.
After this, the Tegra194 SoC data becomes redundant, so get rid of it.
Signed-off-by: Yi-Wei Wang <yiweiw@nvidia.com>
Co-developed-by: Mikko Perttunen <mperttunen@nvidia.com>
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
drivers/pwm/pwm-tegra.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 172063b51d44..759b98b97b6e 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -59,9 +59,6 @@
struct tegra_pwm_soc {
unsigned int num_channels;
-
- /* Maximum IP frequency for given SoCs */
- unsigned long max_frequency;
};
struct tegra_pwm_chip {
@@ -303,7 +300,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
return ret;
/* Set maximum frequency of the IP */
- ret = dev_pm_opp_set_rate(&pdev->dev, pc->soc->max_frequency);
+ ret = dev_pm_opp_set_rate(&pdev->dev, S64_MAX);
if (ret < 0) {
dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
goto put_pm;
@@ -318,7 +315,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
/* Set minimum limit of PWM period for the IP */
pc->min_period_ns =
- (NSEC_PER_SEC / (pc->soc->max_frequency >> PWM_DUTY_WIDTH)) + 1;
+ (NSEC_PER_SEC / (pc->clk_rate >> PWM_DUTY_WIDTH)) + 1;
pc->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
if (IS_ERR(pc->rst)) {
@@ -397,23 +394,16 @@ static int __maybe_unused tegra_pwm_runtime_resume(struct device *dev)
static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
- .max_frequency = 48000000UL,
};
static const struct tegra_pwm_soc tegra186_pwm_soc = {
.num_channels = 1,
- .max_frequency = 102000000UL,
-};
-
-static const struct tegra_pwm_soc tegra194_pwm_soc = {
- .num_channels = 1,
- .max_frequency = 408000000UL,
};
static const struct of_device_id tegra_pwm_of_match[] = {
{ .compatible = "nvidia,tegra20-pwm", .data = &tegra20_pwm_soc },
{ .compatible = "nvidia,tegra186-pwm", .data = &tegra186_pwm_soc },
- { .compatible = "nvidia,tegra194-pwm", .data = &tegra194_pwm_soc },
+ { .compatible = "nvidia,tegra194-pwm", .data = &tegra186_pwm_soc },
{ }
};
MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
--
2.53.0On Mon, Mar 23, 2026 at 11:36:37AM +0900, Mikko Perttunen wrote:
> From: Yi-Wei Wang <yiweiw@nvidia.com>
>
> The clock driving the Tegra PWM IP can be sourced from different parent
> clocks. Hence, let dev_pm_opp_set_rate() set the max clock rate based
> upon the current parent clock that can be specified via device-tree.
>
> After this, the Tegra194 SoC data becomes redundant, so get rid of it.
>
> Signed-off-by: Yi-Wei Wang <yiweiw@nvidia.com>
> Co-developed-by: Mikko Perttunen <mperttunen@nvidia.com>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> drivers/pwm/pwm-tegra.c | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 172063b51d44..759b98b97b6e 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -59,9 +59,6 @@
>
> struct tegra_pwm_soc {
> unsigned int num_channels;
> -
> - /* Maximum IP frequency for given SoCs */
> - unsigned long max_frequency;
> };
>
> struct tegra_pwm_chip {
> @@ -303,7 +300,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> return ret;
>
> /* Set maximum frequency of the IP */
> - ret = dev_pm_opp_set_rate(&pdev->dev, pc->soc->max_frequency);
> + ret = dev_pm_opp_set_rate(&pdev->dev, S64_MAX);
The documentation comment for dev_pm_opp_set_rate() reads:
Device wanting to run at fmax provided by the opp, should have
already rounded to the target OPP's frequency.
I think using S64_MAX is technically fine (assuming there are no issues
with big numbers in that function), but still it feels wrong to use
something simpler than the comment suggests. Am I missing something?
> if (ret < 0) {
> dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
> goto put_pm;
> @@ -318,7 +315,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>
> /* Set minimum limit of PWM period for the IP */
> pc->min_period_ns =
> - (NSEC_PER_SEC / (pc->soc->max_frequency >> PWM_DUTY_WIDTH)) + 1;
> + (NSEC_PER_SEC / (pc->clk_rate >> PWM_DUTY_WIDTH)) + 1;
Orthogonal to this patch: Should this be
DIV_ROUND_UP(NSEC_PER_SEC, pc->clk_rate >> PWM_DUTY_WIDTH)
? Or even
DIV_ROUND_UP(NSEC_PER_SEC < PWM_DUTY_WIDTH, pc->clk_rate);
? (Note, the latter doesn't work as is, as the first parameter has an
overflow, I guess you're still getting my question.)
Best regards
Uwe
On Wednesday, March 25, 2026 1:45 AM Uwe Kleine-König wrote:
> On Mon, Mar 23, 2026 at 11:36:37AM +0900, Mikko Perttunen wrote:
> > From: Yi-Wei Wang <yiweiw@nvidia.com>
> >
> > The clock driving the Tegra PWM IP can be sourced from different parent
> > clocks. Hence, let dev_pm_opp_set_rate() set the max clock rate based
> > upon the current parent clock that can be specified via device-tree.
> >
> > After this, the Tegra194 SoC data becomes redundant, so get rid of it.
> >
> > Signed-off-by: Yi-Wei Wang <yiweiw@nvidia.com>
> > Co-developed-by: Mikko Perttunen <mperttunen@nvidia.com>
> > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> > ---
> >
> > drivers/pwm/pwm-tegra.c | 16 +++-------------
> > 1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index 172063b51d44..759b98b97b6e 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -59,9 +59,6 @@
> >
> > struct tegra_pwm_soc {
> >
> > unsigned int num_channels;
> >
> > -
> > - /* Maximum IP frequency for given SoCs */
> > - unsigned long max_frequency;
> >
> > };
> >
> > struct tegra_pwm_chip {
> >
> > @@ -303,7 +300,7 @@ static int tegra_pwm_probe(struct platform_device
> > *pdev)>
> > return ret;
> >
> > /* Set maximum frequency of the IP */
> >
> > - ret = dev_pm_opp_set_rate(&pdev->dev, pc->soc->max_frequency);
> > + ret = dev_pm_opp_set_rate(&pdev->dev, S64_MAX);
>
> The documentation comment for dev_pm_opp_set_rate() reads:
>
> Device wanting to run at fmax provided by the opp, should have
> already rounded to the target OPP's frequency.
>
> I think using S64_MAX is technically fine (assuming there are no issues
> with big numbers in that function), but still it feels wrong to use
> something simpler than the comment suggests. Am I missing something?
Looking at the history of the function, the comment was added in the commit
below. It seems like it used to be that the opp framework always used the fmax
of each OPP even if a lower rate was specified, but after the change, the
caller has to specify the fmax rate if they want that rate specifically. As
such I don't think it should be an issue in our case, as we're just using the
rate to find an OPP and don't have a specific one in mind.
commit b3e3759ee4abd72bedbf4b109ff1749d3aea6f21
Author: Stephen Boyd <swboyd@chromium.org>
Date: Wed Mar 20 15:19:08 2019 +0530
opp: Don't overwrite rounded clk rate
The OPP table normally contains 'fmax' values corresponding to the
voltage or performance levels of each OPP, but we don't necessarily want
all the devices to run at fmax all the time. Running at fmax makes sense
for devices like CPU/GPU, which have a finite amount of work to do and
since a specific amount of energy is consumed at an OPP, its better to
run at the highest possible frequency for that voltage value.
On the other hand, we have IO devices which need to run at specific
frequencies only for their proper functioning, instead of maximum
possible frequency.
The OPP core currently roundup to the next possible OPP for a frequency
and select the fmax value. To support the IO devices by the OPP core,
lets do the roundup to fetch the voltage or performance state values,
but not use the OPP frequency value. Rather use the value returned by
clk_round_rate().
The current user, cpufreq, of dev_pm_opp_set_rate() already does the
rounding to the next OPP before calling this routine and it won't
have any side affects because of this change.
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
[ Viresh: Massaged changelog, added comment and use temp_opp variable
instead ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> > if (ret < 0) {
> >
> > dev_err(&pdev->dev, "Failed to set max frequency: %d\n",
ret);
> > goto put_pm;
> >
> > @@ -318,7 +315,7 @@ static int tegra_pwm_probe(struct platform_device
> > *pdev)>
> > /* Set minimum limit of PWM period for the IP */
> > pc->min_period_ns =
> >
> > - (NSEC_PER_SEC / (pc->soc->max_frequency >> PWM_DUTY_WIDTH)) + 1;
> > + (NSEC_PER_SEC / (pc->clk_rate >> PWM_DUTY_WIDTH)) + 1;
>
> Orthogonal to this patch: Should this be
>
> DIV_ROUND_UP(NSEC_PER_SEC, pc->clk_rate >> PWM_DUTY_WIDTH)
>
> ? Or even
>
> DIV_ROUND_UP(NSEC_PER_SEC < PWM_DUTY_WIDTH, pc->clk_rate);
>
> ? (Note, the latter doesn't work as is, as the first parameter has an
> overflow, I guess you're still getting my question.)
Indeed, it would be overestimating the minimum period right now. It's not
quite part of Tegra264 support but I can include a patch in the next revision
if you'd like. Otherwise I could include it in the followup series or as a
separate patch.
>
> Best regards
> Uwe
Thanks!
Mikko
[ Adding OPP maintainers to Cc: ]
Helle Mikko,
On Wed, Mar 25, 2026 at 09:34:55AM +0900, Mikko Perttunen wrote:
> On Wednesday, March 25, 2026 1:45 AM Uwe Kleine-König wrote:
> > On Mon, Mar 23, 2026 at 11:36:37AM +0900, Mikko Perttunen wrote:
> > > @@ -303,7 +300,7 @@ static int tegra_pwm_probe(struct platform_device
> > > *pdev)>
> > > return ret;
> > >
> > > /* Set maximum frequency of the IP */
> > >
> > > - ret = dev_pm_opp_set_rate(&pdev->dev, pc->soc->max_frequency);
> > > + ret = dev_pm_opp_set_rate(&pdev->dev, S64_MAX);
> >
> > The documentation comment for dev_pm_opp_set_rate() reads:
> >
> > Device wanting to run at fmax provided by the opp, should have
> > already rounded to the target OPP's frequency.
> >
> > I think using S64_MAX is technically fine (assuming there are no issues
> > with big numbers in that function), but still it feels wrong to use
> > something simpler than the comment suggests. Am I missing something?
>
> Looking at the history of the function, the comment was added in the commit
> below. It seems like it used to be that the opp framework always used the fmax
> of each OPP even if a lower rate was specified, but after the change, the
> caller has to specify the fmax rate if they want that rate specifically. As
> such I don't think it should be an issue in our case, as we're just using the
> rate to find an OPP and don't have a specific one in mind.
>
> commit b3e3759ee4abd72bedbf4b109ff1749d3aea6f21
> Author: Stephen Boyd <swboyd@chromium.org>
> Date: Wed Mar 20 15:19:08 2019 +0530
>
> opp: Don't overwrite rounded clk rate
>
> The OPP table normally contains 'fmax' values corresponding to the
> voltage or performance levels of each OPP, but we don't necessarily want
> all the devices to run at fmax all the time. Running at fmax makes sense
> for devices like CPU/GPU, which have a finite amount of work to do and
> since a specific amount of energy is consumed at an OPP, its better to
> run at the highest possible frequency for that voltage value.
>
> On the other hand, we have IO devices which need to run at specific
> frequencies only for their proper functioning, instead of maximum
> possible frequency.
>
> The OPP core currently roundup to the next possible OPP for a frequency
> and select the fmax value. To support the IO devices by the OPP core,
> lets do the roundup to fetch the voltage or performance state values,
> but not use the OPP frequency value. Rather use the value returned by
> clk_round_rate().
>
> The current user, cpufreq, of dev_pm_opp_set_rate() already does the
> rounding to the next OPP before calling this routine and it won't
> have any side affects because of this change.
>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> [ Viresh: Massaged changelog, added comment and use temp_opp variable
> instead ]
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
So the comment describing dev_pm_opp_set_rate() needs an update, right?
> > > if (ret < 0) {
> > >
> > > dev_err(&pdev->dev, "Failed to set max frequency: %d\n",
> ret);
> > > goto put_pm;
> > >
> > > @@ -318,7 +315,7 @@ static int tegra_pwm_probe(struct platform_device
> > > *pdev)>
> > > /* Set minimum limit of PWM period for the IP */
> > > pc->min_period_ns =
> > >
> > > - (NSEC_PER_SEC / (pc->soc->max_frequency >> PWM_DUTY_WIDTH)) + 1;
> > > + (NSEC_PER_SEC / (pc->clk_rate >> PWM_DUTY_WIDTH)) + 1;
> >
> > Orthogonal to this patch: Should this be
> >
> > DIV_ROUND_UP(NSEC_PER_SEC, pc->clk_rate >> PWM_DUTY_WIDTH)
> >
> > ? Or even
> >
> > DIV_ROUND_UP(NSEC_PER_SEC < PWM_DUTY_WIDTH, pc->clk_rate);
> >
> > ? (Note, the latter doesn't work as is, as the first parameter has an
> > overflow, I guess you're still getting my question.)
>
> Indeed, it would be overestimating the minimum period right now. It's not
> quite part of Tegra264 support but I can include a patch in the next revision
> if you'd like. Otherwise I could include it in the followup series or as a
> separate patch.
If you know it and feel responsible to address it at some point that's
fine. We lived with that issue for some time now, so a separate and if
you prefer later series is fine for me.
Best regards
Uwe
On 25-03-26, 07:12, Uwe Kleine-König wrote: > On Wed, Mar 25, 2026 at 09:34:55AM +0900, Mikko Perttunen wrote: > > On Wednesday, March 25, 2026 1:45 AM Uwe Kleine-König wrote: > > > On Mon, Mar 23, 2026 at 11:36:37AM +0900, Mikko Perttunen wrote: > > > > @@ -303,7 +300,7 @@ static int tegra_pwm_probe(struct platform_device > > > > *pdev)> > > > > return ret; > > > > > > > > /* Set maximum frequency of the IP */ > > > > > > > > - ret = dev_pm_opp_set_rate(&pdev->dev, pc->soc->max_frequency); > > > > + ret = dev_pm_opp_set_rate(&pdev->dev, S64_MAX); > > > > > > The documentation comment for dev_pm_opp_set_rate() reads: > > > > > > Device wanting to run at fmax provided by the opp, should have > > > already rounded to the target OPP's frequency. And that is correct, right ? This comment is talking about the max freq possible with each OPP and not the highest freq possible with the device. If a device supports 5 OPPs (0-4) and if we want to run at the freq mentioned in the OPP3 entry in DT, then the caller must send a frequency such that clk_round_rate() returns the frequency of the OPP3. In the above case though, we will end up running at the highest freq returned by clk_round_rate() and an OPP corresponding to that. > > > I think using S64_MAX is technically fine (assuming there are no issues > > > with big numbers in that function), but still it feels wrong to use > > > something simpler than the comment suggests. Am I missing something? I think S64_MAX will work as well, unless clk_round_rate() returns a frequency higher than what the OPP table mentions. It may still work, but the values may be confusing and inconsistent. > > Looking at the history of the function, the comment was added in the commit > > below. It seems like it used to be that the opp framework always used the fmax > > of each OPP even if a lower rate was specified, but after the change, the > > caller has to specify the fmax rate if they want that rate specifically. As > > such I don't think it should be an issue in our case, as we're just using the > > rate to find an OPP and don't have a specific one in mind. Right. > So the comment describing dev_pm_opp_set_rate() needs an update, right? Maybe, not sure. But as I mentioned earlier, it is written with the context of each OPP's highest freq. -- viresh
© 2016 - 2026 Red Hat, Inc.