| Message ID | 20221018-clk-range-checks-fixes-v4-7-971d5077e7d2@cerno.tech (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: Make determine_rate mandatory for muxes | expand |
Quoting Maxime Ripard (2023-05-05 04:25:09) > The single parent clock in our kunit tests implements a mux with a > set_parent hook, but doesn't provide a determine_rate implementation. > > This is not entirely unexpected, since its whole purpose it to have a > single parent. When determine_rate is missing, and since > CLK_SET_RATE_PARENT is set for all its instances, the default behaviour > of the framework will be to forward it to the current parent. > > This is totally fine as far as the tests are concerned, but we'll start > to mandate a determine_rate implementation when set_parent is set, so > let's fill it with __clk_mux_determine_rate() which will have the same > behavior. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/clk/clk_test.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c > index b3ed3b0e4c31..a154ec9d0111 100644 > --- a/drivers/clk/clk_test.c > +++ b/drivers/clk/clk_test.c > @@ -104,6 +104,23 @@ static const struct clk_ops clk_dummy_minimize_rate_ops = { > }; > > static const struct clk_ops clk_dummy_single_parent_ops = { > + /* > + * FIXME: Even though we should probably be able to use Are we ever going to fix this? > + * __clk_mux_determine_rate() here, if we use it and call > + * clk_round_rate() or clk_set_rate() with a rate lower than > + * what all the parents can provide, it will return -EINVAL. > + * > + * This is due to the fact that it has the undocumented > + * behaviour to always pick up the closest rate higher than the > + * requested rate. If we get something lower, it thus considers > + * that it's not acceptable and will return an error. > + * > + * It's somewhat inconsistent and creates a weird threshold > + * between rates above the parent rate which would be rounded to > + * what the parent can provide, but rates below will simply > + * return an error. > + */ > + .determine_rate = __clk_mux_determine_rate_closest, > .set_parent = clk_dummy_single_set_parent, > .get_parent = clk_dummy_single_get_parent,
Hi, On Thu, Jun 08, 2023 at 06:41:56PM -0700, Stephen Boyd wrote: > Quoting Maxime Ripard (2023-05-05 04:25:09) > > The single parent clock in our kunit tests implements a mux with a > > set_parent hook, but doesn't provide a determine_rate implementation. > > > > This is not entirely unexpected, since its whole purpose it to have a > > single parent. When determine_rate is missing, and since > > CLK_SET_RATE_PARENT is set for all its instances, the default behaviour > > of the framework will be to forward it to the current parent. > > > > This is totally fine as far as the tests are concerned, but we'll start > > to mandate a determine_rate implementation when set_parent is set, so > > let's fill it with __clk_mux_determine_rate() which will have the same > > behavior. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/clk/clk_test.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c > > index b3ed3b0e4c31..a154ec9d0111 100644 > > --- a/drivers/clk/clk_test.c > > +++ b/drivers/clk/clk_test.c > > @@ -104,6 +104,23 @@ static const struct clk_ops clk_dummy_minimize_rate_ops = { > > }; > > > > static const struct clk_ops clk_dummy_single_parent_ops = { > > + /* > > + * FIXME: Even though we should probably be able to use > > Are we ever going to fix this? We probably should, but I'm not entirely sure how. > > + * __clk_mux_determine_rate() here, if we use it and call > > + * clk_round_rate() or clk_set_rate() with a rate lower than > > + * what all the parents can provide, it will return -EINVAL. > > + * > > + * This is due to the fact that it has the undocumented > > + * behaviour to always pick up the closest rate higher than the > > + * requested rate. If we get something lower, it thus considers > > + * that it's not acceptable and will return an error. > > + * > > + * It's somewhat inconsistent and creates a weird threshold > > + * between rates above the parent rate which would be rounded to > > + * what the parent can provide, but rates below will simply > > + * return an error. > > + */ I guess it's mostly a policy decision: __clk_mux_determine_rate() always has been returning rates that are lower or equal to the target rate. If no parent can provide one, then the obvious solution is to error out, which creates the inconsistency mentioned above. Another solution would be to pick up a parent by default (the current one maybe?) but then we could return a rate higher than the target rate which breaks what we've been doing so far. I'm not sure if one is better than the other. Maxime
Quoting Maxime Ripard (2023-06-13 01:21:59) > On Thu, Jun 08, 2023 at 06:41:56PM -0700, Stephen Boyd wrote: > > Quoting Maxime Ripard (2023-05-05 04:25:09) > > > > + * __clk_mux_determine_rate() here, if we use it and call > > > + * clk_round_rate() or clk_set_rate() with a rate lower than > > > + * what all the parents can provide, it will return -EINVAL. > > > + * > > > + * This is due to the fact that it has the undocumented > > > + * behaviour to always pick up the closest rate higher than the > > > + * requested rate. If we get something lower, it thus considers > > > + * that it's not acceptable and will return an error. > > > + * > > > + * It's somewhat inconsistent and creates a weird threshold > > > + * between rates above the parent rate which would be rounded to > > > + * what the parent can provide, but rates below will simply > > > + * return an error. > > > + */ > > I guess it's mostly a policy decision: __clk_mux_determine_rate() always > has been returning rates that are lower or equal to the target rate. > > If no parent can provide one, then the obvious solution is to error out, > which creates the inconsistency mentioned above. > > Another solution would be to pick up a parent by default (the current > one maybe?) but then we could return a rate higher than the target rate > which breaks what we've been doing so far. > > I'm not sure if one is better than the other. We should pick a rounding policy that we want for the test. The test shouldn't be checking the rounding policy. It should be checking that the code under test does what is expected when the rounding policy is what we choose. If __clk_mux_determine_rate() returns an error when the rate is lower than the parent supports then we should test that as well and make sure it does return an error in this case. We can directly call __clk_mux_determine_rate() after registering the clks so that the function is isolated. We should also be testing what the clk API will do if a determine_rate() clk_op returns an error. And what it will do if the clk has multiple parents, etc. I'm mostly confused that it's marked as a FIXME when the test should be doing wacky things to test various pieces of code.
Hi Stephen, On Tue, Jun 13, 2023 at 11:39:58AM -0700, Stephen Boyd wrote: > Quoting Maxime Ripard (2023-06-13 01:21:59) > > On Thu, Jun 08, 2023 at 06:41:56PM -0700, Stephen Boyd wrote: > > > Quoting Maxime Ripard (2023-05-05 04:25:09) > > > > > > + * __clk_mux_determine_rate() here, if we use it and call > > > > + * clk_round_rate() or clk_set_rate() with a rate lower than > > > > + * what all the parents can provide, it will return -EINVAL. > > > > + * > > > > + * This is due to the fact that it has the undocumented > > > > + * behaviour to always pick up the closest rate higher than the > > > > + * requested rate. If we get something lower, it thus considers > > > > + * that it's not acceptable and will return an error. > > > > + * > > > > + * It's somewhat inconsistent and creates a weird threshold > > > > + * between rates above the parent rate which would be rounded to > > > > + * what the parent can provide, but rates below will simply > > > > + * return an error. > > > > + */ > > > > I guess it's mostly a policy decision: __clk_mux_determine_rate() always > > has been returning rates that are lower or equal to the target rate. > > > > If no parent can provide one, then the obvious solution is to error out, > > which creates the inconsistency mentioned above. > > > > Another solution would be to pick up a parent by default (the current > > one maybe?) but then we could return a rate higher than the target rate > > which breaks what we've been doing so far. > > > > I'm not sure if one is better than the other. > > We should pick a rounding policy that we want for the test. The test > shouldn't be checking the rounding policy. It should be checking that > the code under test does what is expected when the rounding policy is > what we choose. > > If __clk_mux_determine_rate() returns an error when the rate is lower > than the parent supports then we should test that as well and make sure > it does return an error in this case. We can directly call > __clk_mux_determine_rate() after registering the clks so that the > function is isolated. I guess my point is that, in general, we don't really expect an error in clk_round_rate(). Most clocks will clamp the rates between the minimum and maximum allowed and will select the closest parent or dividers to match that rate. So we could end up with a rate fairly different from the argument, but we'll get a rate. For muxes using __clk_mux_determine_rate*() without CLK_SET_RATE_PARENT, if the rate is too low (or if the range is loo low) and is right below what one of the parent can provide, we end up with -EINVAL. AFAIK, it's pretty much the only situation where that happens. A fixed clock will always return its fixed rate for example, even if the rate is very different from what we asked for. But for some reason, on some muxes, with some rates, boom, EINVAL. It's very unexpected and inconsistent to me. Hence the FIXME. Maxime
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c index b3ed3b0e4c31..a154ec9d0111 100644 --- a/drivers/clk/clk_test.c +++ b/drivers/clk/clk_test.c @@ -104,6 +104,23 @@ static const struct clk_ops clk_dummy_minimize_rate_ops = { }; static const struct clk_ops clk_dummy_single_parent_ops = { + /* + * FIXME: Even though we should probably be able to use + * __clk_mux_determine_rate() here, if we use it and call + * clk_round_rate() or clk_set_rate() with a rate lower than + * what all the parents can provide, it will return -EINVAL. + * + * This is due to the fact that it has the undocumented + * behaviour to always pick up the closest rate higher than the + * requested rate. If we get something lower, it thus considers + * that it's not acceptable and will return an error. + * + * It's somewhat inconsistent and creates a weird threshold + * between rates above the parent rate which would be rounded to + * what the parent can provide, but rates below will simply + * return an error. + */ + .determine_rate = __clk_mux_determine_rate_closest, .set_parent = clk_dummy_single_set_parent, .get_parent = clk_dummy_single_get_parent, };
The single parent clock in our kunit tests implements a mux with a set_parent hook, but doesn't provide a determine_rate implementation. This is not entirely unexpected, since its whole purpose it to have a single parent. When determine_rate is missing, and since CLK_SET_RATE_PARENT is set for all its instances, the default behaviour of the framework will be to forward it to the current parent. This is totally fine as far as the tests are concerned, but we'll start to mandate a determine_rate implementation when set_parent is set, so let's fill it with __clk_mux_determine_rate() which will have the same behavior. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/clk_test.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)