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

[Thierry Reding <thierry.reding@gmail.com>]

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

On Thu, Sep 22, 2022 at 12:12:31PM +0100, Jon Hunter wrote:
> 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";
>         };

Traditionally we've always set the clock parent in the driver via the
init table (at least on chips prior to Tegra186). On the other hand we
have a few occurrences of assigned-clock-rates already for Tegra210. I
don't feel strongly either way. A minor advantage of having the fix in
the clock driver is that it fixes this automatically for older device
trees. Not that it really matters since people always update kernel and
DTB at the same time on Tegra devices.

It'd be great if we could get confirmation that changing the parent
clock fixes this on all other boards as well, then we can fix it at the
same time for all of them.

Thierry

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