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
