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

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 [off-list ref]

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.

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

[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