Re: [PATCH v2] pwm: tegra: Optimize period calculation

[Jon Hunter <jonathanh@nvidia.com>]

Hi Uwe,

On 21/09/2022 09:17, Uwe Kleine-König wrote:

...

> As the clk-rate is only 32768 Hz we get (with period_ns = 1000000)
> 
> 	32768 * 1000000 / (1000000000 << 8) = 0.128
> 
> which is indeed rounded down to 0 and then runs into the error path
> returning -EINVAL. Before my change (that now broke the backlight
> configuration) configuration continued and then ended with actually
> configuring period = 7812500 ns which is off by nearly a factor of 8.

I am seeing the same issue on Tegra210 Jetson Nano (device-tree
tegra210-p3450-0000.dts). This also has a clock rate of 32768 Hz by
default which means the min period is 30517ns. However, in the probe
the min_period_ns comes from the pc->soc->max_frequency which is 48
MHz for Tegra210. The min_period_ns = 1/(48 MHz / (2^8)) which is
5334ns. Hence, the actual min period is less than what is actually
possible.

I wonder if we should be warning about this and fixing the min
period ...

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 2f3dcb9e9278..f72928c05c81 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -310,9 +310,13 @@ static int tegra_pwm_probe(struct platform_device *pdev)
          */
         pc->clk_rate = clk_get_rate(pc->clk);
  
+       if (pc->clk_rate < pc->soc->max_frequency)
+               dev_warn(&pdev->dev, "Max frequency limited to %lu Hz!",
+                        pc->clk_rate);
+
         /* 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");

The above does not fix this issue but ...
  
> I didn't find a device tree for an Asus Transformer tablet bases on a
> tegra124 in the kernel source, but the options are:
> 
>   - Revert commit 8c193f4714df ("pwm: tegra: Optimize period calculation").
>     I don't like this. IMHO this commit is an improvement and the problem
>     is that the consumer requests a too small period. For a backlight
>     this might be ok to result in a much bigger period, for other
>     usecases it isn't and so I like refusing period = 1000000.
> 
>   - We could just drop the "else / return -EINVAL".
>     This is inconsistent as then (again) some periods are rounded up
>     (with the given clk rate that would be 5334 <= period < 7812500)
>     while others (period < 5334) yield -EINVAL.
> 
>   - Increase the period that the backlight is using to at least 7812500.
>     This is done (I guess) by replacing 1000000 by 7812500 (or more) in
>     the backlight's PWM phandle.
> 
>   - Make the PWM clk faster.
>     Looking quickly through the tegra clk driver, the parent of the PWM
>     clk could be changed from clk_32k to pll_p or pll_c. This should be
>     doable in the dts using something like:
> 
>     	assigned-clocks = <&tegra_car TEGRA124_CLK_PWM>;
> 	assigned-clock-parents = <&tegra_car TEGRA124_CLK_PLL_P>;
> 
>     in the pwm node. (Note this includes much guesswork, I don't know the
>     PPL's clk-rate, so this might break in other ways. Another option is
>     using PLL_C.)
> 
> Probably the latter is the nicest option. Is it possible to find out the
> setting when the machine is running the original vendor OS?

The latter does seem correct to me. This fixes the issue for Tegra210 ...

diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index 4f0e51f1a343..842843e0a585 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -670,6 +670,10 @@
                 clock-names = "pwm";
                 resets = <&tegra_car 17>;
                 reset-names = "pwm";
+
+               assigned-clocks = <&tegra_car TEGRA210_CLK_PWM>;
+               assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_P>;
+
                 status = "disabled";
         };

Cheers
Jon

-- 
nvpublic