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 isAs 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
