On 26/10/2022 01:10, Uwe Kleine-König wrote:
...
2. Even after fixing issue #1, above, I then ran into another issue
where even if I request a clock rate of 5646975 I still get a lower
clock. This also causes mul_u64_u64_div_u64() to return 0 and then I
see the -EINVAL returned. The simple solution here is to not return
-EINVAL for 0. After all 0, could be valid if the rate if we are
not dividing down the parent clock in the PWM. However, then there is
As you have to subtract 1 from the result of the division, you need to
write a -1 in the register which doesn't work. Writing a 0 results in a
bigger period than requested which is the thing I intended to fix with
the blamed commit.
If clk_rate was 5646976 (don't know if that matches as nicely as it
should? If dev_pm_opp_set_rate might set a lower rate you're out of luck
again) we get:
rate = mul_u64_u64_div_u64(5646976, 45334, 1000000000 << 8)
which is 1 and everything is fine.
Note that the math before my commit already had that problem. Before the
driver was just more lax and didn't subtract 1 from rate and so
configured a bigger period than requested/expected
There are probably similar cases where the driver still configures a too
big period (i.e. when the effect you described yields a rate that is too
small but bigger than 0).
So the optimal way to fix this is to adapt the calculation of
required_clk_rate as you suggested + rounding up and to make sure that
dev_pm_opp_set_rate doesn't configure a rate lower than requested. The
latter might be complicated as the API (AFAIK) isn't specific about
rounding directions and there isn't a dev_pm_opp_round_rate function.
As a short term work-around dropping the -EINVAL is probably best, I'd
like to have it documented clearly however that continuing in that case
is wrong and yields unexpected settings.
I have done a bit more testing today and sent out a couple fixes. I have
added you to the review. Let me know what you think.
Jon
--
nvpublic