Hello Thomas,
On Tue, Oct 04, 2022 at 12:28:25PM +0200, Thomas Graichen wrote:
> On Fri, Sep 23, 2022 at 2:10 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> >
> > 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
>
> just a little update: i just compiled a v6.0 kernel for the nyan
> chromebook (tegra124) and now the display remains completely black -
> with v5.19 it was at least working with broken brightness control -
> reverting the pwm optimization patch it works perfectly fine in both
> cases ... please let me know if you have any patches i should test to
> fix this
You might want to test
https://lore.kernel.org/linux-clk/20221003101555.25458-1-jonathanh@xxxxxxxxxx
(Hmm, tegra124 != tegra210? Then something similar should be done for
tegra124?)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
