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
best wishes - thomas
