Re: [PATCH v6 3/7] pwm: tegra: Avoid hard-coded max clock frequency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] dev_pm_opp_set_rate(..., ULONG_MAX) causes probe failure when the parent clock's maximum rate exceeds the DT OPP table limits.
- [High] Dropping the explicit max_frequency cap permanently locks older SoCs (num_channels > 1) to a high clock rate, severely limiting the maximum PWM period.

Pre-existing issues:
- [High] Missing pm_runtime_disable() on probe error paths and in driver remove.
--

commit 29d7ce61a02c025b3580d289a189425cd7da22aa
Author: Yi-Wei Wang <yiweiw@nvidia.com>

pwm: tegra: Avoid hard-coded max clock frequency

The patch removes the hard-coded maximum frequency for Tegra PWM SoCs.
It updates probe to use dev_pm_opp_set_rate with ULONG_MAX to set the clock
rate based on the parent clock limits from the device tree. It also removes
the now redundant tegra194_pwm_soc configuration.

> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index f8c5495a56a66..3eef299c51f8e 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, ULONG_MAX);

[Severity: High]
If the parent clock's maximum physical rate exceeds the maximum frequency
defined in the device tree OPP table, will dev_pm_opp_set_rate() fail here?

The OPP core translates ULONG_MAX using clk_round_rate() to find the absolute
maximum frequency the parent clock can physically generate. If this is higher
than the maximum frequency in the OPP table (for example, on Tegra30),
_find_freq_ceil() might fail to find a valid OPP and return -ERANGE, causing
the probe to fail.

Also, for older SoCs where num_channels > 1, tegra_pwm_config() does not
dynamically scale the clock rate.

drivers/pwm/pwm-tegra.c:tegra_pwm_config() {
    ...
    if (pc->soc->num_channels == 1) {
        ...
        err = dev_pm_opp_set_rate(pwmchip_parent(chip), required_clk_rate);
        ...
    }
    
    /* Consider precision in scale_width rate calculation */
    rate = mul_u64_u64_div_u64(pc->clk_rate, period_ns,
                               (u64)NSEC_PER_SEC * TEGRA_PWM_DEPTH);
    ...
    if (rate >> pc->soc->scale_width)
        return -EINVAL;
    ...
}

By forcing the clock to its absolute maximum frequency during probe, won't
these older SoCs be permanently locked to a high clock rate?

With a high fixed clock rate, calculating the divider (rate) for large requested
periods (like 20ms for servos) could exceed the hardware's scale_width limit,
causing legitimate config requests to fail with -EINVAL.

>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
>  		goto put_pm;
>  	}
> @@ -315,10 +312,15 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	 * so that PWM period can be calculated more accurately.
>  	 */
>  	pc->clk_rate = clk_get_rate(pc->clk);
> +	if (pc->clk_rate < (1 << TEGRA_PWM_DUTY_WIDTH)) {
> +		dev_err(&pdev->dev, "clock maximum frequency out of range\n");
> +		ret = -ERANGE;
> +		goto put_pm;

[Severity: High]
This is a pre-existing issue, but does this error path leave the runtime PM
state unbalanced?

The driver calls pm_runtime_enable() earlier in tegra_pwm_probe(), but the error
handling at the put_pm label (and similarly the tegra_pwm_remove function) only
calls suspend functions without calling pm_runtime_disable():

drivers/pwm/pwm-tegra.c:tegra_pwm_probe() {
    ...
put_pm:
    pm_runtime_put_sync_suspend(&pdev->dev);
    pm_runtime_force_suspend(&pdev->dev);
    return ret;
}

Could this lead to state corruption or kernel warnings upon module reload or
device re-bind since the enable is never balanced?

> +	}
>  
>  	/* Set minimum limit of PWM period for the IP */
>  	pc->min_period_ns =
> -	    (NSEC_PER_SEC / (pc->soc->max_frequency >> TEGRA_PWM_DUTY_WIDTH)) + 1;
> +	    (NSEC_PER_SEC / (pc->clk_rate >> TEGRA_PWM_DUTY_WIDTH)) + 1;
>  
>  	pc->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
>  	if (IS_ERR(pc->rst)) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-t264-pwm-v6-0-2718f61f411f@nvidia.com?part=3