Re: [PATCH 1/5] pwm: tegra: Avoid hard-coded max clock frequency

Re: [PATCH 1/5] pwm: tegra: Avoid hard-coded max clock frequency

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4525 bytes --]

[ 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] pwm: tegra: Avoid hard-coded max clock frequency

[Viresh Kumar]

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