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

[Mikko Perttunen]

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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > Co-developed-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > ---
> > 
> >  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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
    Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
    [ Viresh: Massaged changelog, added comment and use temp_opp variable
              instead ]
    Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>


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