Hi, On Thu, Feb 24, 2022 at 02:32:37PM -0800, Stephen Boyd wrote: > Quoting Maxime Ripard (2022-02-21 08:18:21) > > Hi, > > > > On Fri, Feb 18, 2022 at 03:15:06PM -0800, Stephen Boyd wrote: > > > Quoting Maxime Ripard (2022-01-25 06:15:41) > > > > The current core while setting the min and max rate properly in the > > > > clk_request structure will not make sure that the requested rate is > > > > within these boundaries, leaving it to each and every driver to make > > > > sure it is. > > > > > > It would be good to describe why. Or decide that it was an oversight and > > > write that down here. > > > > > > > Add a clamp call to make sure it's always done, and add a few unit tests > > > > to make sure we don't have any regression there. > > > > > > I looked through the per-user constraint patch history on the list but I > > > couldn't really figure out why it was done this way. I guess we didn't > > > clamp the rate in the core because we wanted to give the clk providers > > > all the information, i.e. the rate that was requested and the boundaries > > > that the consumers have placed on the rate. > > > > I'm not really sure we should really leave it to the users, something like: > > > > clk_set_range_rate(clk, 1000, 2000); > > clk_set_rate(clk, 500); > > clk_get_rate(clk) # == 500 > > > > Is definitely weird, and would break the least surprise :) > > > > We shouldn't leave that to drivers, especially since close to none of > > them handle this properly. > > Ok. > > > > With the round_rate() clk_op the providers don't know the min/max > > > because the rate request structure isn't passed. I think my concern a > > > long time ago was that a consumer could call clk_round_rate() and get > > > one frequency and then call clk_set_rate() and get another frequency. > > > > I'm not sure I follow you there. > > > > The function affected is clk_core_determine_round_nolock(), which is > > called by clk_core_round_rate_nolock() and clk_calc_new_rates(). In > > turn, they will be part of clk(_hw_)_round_clock for the former, and > > clk_core_set_rate_nolock() (and thus clk_set_rate()) for the latter. > > > > I don't see how you can get a discrepancy between clk_round_rate() and > > clk_set_rate(). > > > > And yeah, it's true that the round_rate op won't have the min and max > > passed to them, but i'd consider this an argument for doing this check > > here, since you don't have that option at all for those clocks. > > When the range setting API was introduced the rounding logic and the > rate setting logic didn't use the same code paths. It looks like that > code got consolidated now though so we should be fine. Actually, there was a discrepancy. If you are doing, before this patch series: clk_set_range_rate(clk, 1000, 2000) clk_round_rate(500); Unless the driver was involved, the returned rate would be 500. Now, if you call clk_set_rate(500), it will return -EINVAL, hitting the check here: https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1973 If the driver was looking at the min and max and clamping the rate, you would get clk_round_rate() == 1000 and clk_set_rate() would succeed, with the rate set to 1000. This seems like an abstraction leakage to me. This patch fixes that discrepancy, but in the last version I sent, I added a test that would check that once you have a range in place, then clk_round_rate and clk_set_rate/clk_get_rate would return the same value. Maxime
Attachment:
signature.asc
Description: PGP signature
