| Message ID | 20221018-clk-range-checks-fixes-v2-43-f6736dec138e@cerno.tech |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clk: Make determine_rate mandatory for muxes | expand |
Hi Mark, On Fri, Nov 04, 2022 at 03:44:53PM +0000, Mark Brown wrote: > On Fri, Nov 04, 2022 at 02:18:00PM +0100, Maxime Ripard wrote: > > > So, the set_parent hook is effectively unused, possibly because of an > > oversight. However, it could also be an explicit decision by the > > original author to avoid any reparenting but through an explicit call to > > clk_set_parent(). > > > The latter case would be equivalent to setting the flag > > CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook > > to __clk_mux_determine_rate(). Indeed, if no determine_rate > > implementation is provided, clk_round_rate() (through > > clk_core_round_rate_nolock()) will call itself on the parent if > > CLK_SET_RATE_PARENT is set, and will not change the clock rate > > otherwise. __clk_mux_determine_rate() has the exact same behavior when > > CLK_SET_RATE_NO_REPARENT is set. > > > And if it was an oversight, then we are at least explicit about our > > behavior now and it can be further refined down the line. > > Given that the current approach involves patching every single user to > set a default implementation it feels like it might be more > straightforward to just have the clock API use that implementation if > none is defined - as you say there's already a flag to indicate the > unusual case where there's a solid reason to prevent reparenting. It > feels like the resulting API is more straightforward. That would be another solution indeed. The thing is, most places where determine_rate is missing seems to be oversight, and the flag is missing as well. Just filling determine_rate if it's missing with __clk_mux_determine_rate will possibly pick different parents, and I'm fairly certain that this have never been tested on most platforms, and will be completely broken. And I don't really want to play a game of whack-a-mole adding that flag everywhere it turns out it's broken. Maxime
Hi Mark, On Fri, Nov 04, 2022 at 03:59:53PM +0000, Mark Brown wrote: > On Fri, Nov 04, 2022 at 04:51:23PM +0100, Maxime Ripard wrote: > > > Just filling determine_rate if it's missing with > > __clk_mux_determine_rate will possibly pick different parents, and I'm > > fairly certain that this have never been tested on most platforms, and > > will be completely broken. And I don't really want to play a game of > > whack-a-mole adding that flag everywhere it turns out it's broken. > > Well, hopefully everyone for whom it's an issue currently will be > objecting to this version of the change anyway so we'll either know > where to set the flag or we'll get the whack-a-mole with the series > being merged? I'm sorry, I'm not sure what you mean here. The only issue to fix at the moment is that determine_rate and set_parent aren't coupled, and it led to issues due to oversight. I initially added a warning but Stephen wanted to fix all users in that case and make that an error instead. If I filled __clk_mux_determine_rate into clocks that weren't using it before, I would change their behavior. With that flag set, on all users I add __clk_mux_determine_rate to, the behavior is the same than what we previously had, so the risk of regressions is minimal, and everything should keep going like it was? Maxime
On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote: > On Fri, Nov 04, 2022 at 03:59:53PM +0000, Mark Brown wrote: > > Well, hopefully everyone for whom it's an issue currently will be > > objecting to this version of the change anyway so we'll either know > > where to set the flag or we'll get the whack-a-mole with the series > > being merged? > I'm sorry, I'm not sure what you mean here. The only issue to fix at the > moment is that determine_rate and set_parent aren't coupled, and it led > to issues due to oversight. > I initially added a warning but Stephen wanted to fix all users in that > case and make that an error instead. My suggestion is that instead of doing either of these things it'd be quicker and less error prone to just fix the core to provide the default implementation if nothing more specific is provided. Any issues that causes would already be present with your current series. > If I filled __clk_mux_determine_rate into clocks that weren't using it > before, I would change their behavior. With that flag set, on all users > I add __clk_mux_determine_rate to, the behavior is the same than what we > previously had, so the risk of regressions is minimal, and everything > should keep going like it was? The series does fill in __clk_mux_determine_rate for everything though - if it was just assumed by default the only thing that'd be needed would be adding the flag.
On Mon, Nov 07, 2022 at 12:06:07PM +0000, Mark Brown wrote: > On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote: > > On Fri, Nov 04, 2022 at 03:59:53PM +0000, Mark Brown wrote: > > > > Well, hopefully everyone for whom it's an issue currently will be > > > objecting to this version of the change anyway so we'll either know > > > where to set the flag or we'll get the whack-a-mole with the series > > > being merged? > > > I'm sorry, I'm not sure what you mean here. The only issue to fix at the > > moment is that determine_rate and set_parent aren't coupled, and it led > > to issues due to oversight. > > > I initially added a warning but Stephen wanted to fix all users in that > > case and make that an error instead. > > My suggestion is that instead of doing either of these things it'd be > quicker and less error prone to just fix the core to provide the default > implementation if nothing more specific is provided. Any issues that > causes would already be present with your current series. > > > If I filled __clk_mux_determine_rate into clocks that weren't using it > > before, I would change their behavior. With that flag set, on all users > > I add __clk_mux_determine_rate to, the behavior is the same than what we > > previously had, so the risk of regressions is minimal, and everything > > should keep going like it was? > > The series does fill in __clk_mux_determine_rate for everything though - > if it was just assumed by default the only thing that'd be needed would > be adding the flag. The behavior assumed by default was equivalent to __clk_mux_determine_rate + CLK_SET_RATE_NO_REPARENT. We could indeed set both if determine_rate is missing in the core, but that's unprecedented in the clock framework so I think we'll want Stephen to comment here :) It's also replacing one implicit behavior by another. The point of this series was to raise awareness on that particular point, so I'm not sure it actually fixes things. We'll see what Stephen thinks about it. Maxime
On Mon, Nov 07, 2022 at 04:26:03PM +0100, Maxime Ripard wrote: > On Mon, Nov 07, 2022 at 12:06:07PM +0000, Mark Brown wrote: > > On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote: > > The series does fill in __clk_mux_determine_rate for everything though - > > if it was just assumed by default the only thing that'd be needed would > > be adding the flag. > The behavior assumed by default was equivalent to > __clk_mux_determine_rate + CLK_SET_RATE_NO_REPARENT. We could indeed set > both if determine_rate is missing in the core, but that's unprecedented > in the clock framework so I think we'll want Stephen to comment here :) > It's also replacing one implicit behavior by another. The point of this > series was to raise awareness on that particular point, so I'm not sure > it actually fixes things. We'll see what Stephen thinks about it. We could also just set the operation and still require the flag to be specified. I'm a little surprised to learn that it's something you might want to override, never mind that the API didn't have a default - it feels like a bit of a landmine that this is the case and is probably why there's so many cases to fix up.
Quoting Maxime Ripard (2022-11-07 07:26:03) > On Mon, Nov 07, 2022 at 12:06:07PM +0000, Mark Brown wrote: > > On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote: > > > On Fri, Nov 04, 2022 at 03:59:53PM +0000, Mark Brown wrote: > > > > > > Well, hopefully everyone for whom it's an issue currently will be > > > > objecting to this version of the change anyway so we'll either know > > > > where to set the flag or we'll get the whack-a-mole with the series > > > > being merged? > > > > > I'm sorry, I'm not sure what you mean here. The only issue to fix at the > > > moment is that determine_rate and set_parent aren't coupled, and it led > > > to issues due to oversight. > > > > > I initially added a warning but Stephen wanted to fix all users in that > > > case and make that an error instead. > > > > My suggestion is that instead of doing either of these things it'd be > > quicker and less error prone to just fix the core to provide the default > > implementation if nothing more specific is provided. Any issues that > > causes would already be present with your current series. > > > > > If I filled __clk_mux_determine_rate into clocks that weren't using it > > > before, I would change their behavior. With that flag set, on all users > > > I add __clk_mux_determine_rate to, the behavior is the same than what we > > > previously had, so the risk of regressions is minimal, and everything > > > should keep going like it was? > > > > The series does fill in __clk_mux_determine_rate for everything though - > > if it was just assumed by default the only thing that'd be needed would > > be adding the flag. > > The behavior assumed by default was equivalent to > __clk_mux_determine_rate + CLK_SET_RATE_NO_REPARENT. We could indeed set > both if determine_rate is missing in the core, but that's unprecedented > in the clock framework so I think we'll want Stephen to comment here :) The clk_ops pointer is const (no writeable jump tables) so we'd have to copy the clk_ops struct on registration to set the __clk_mux_determine_rate() op. We could set the flag though and then check for the absence of a determine_rate op. Things like clk_core_can_round() would need to check for the flag. I'd actually forgotten about this flag. In hindsight I think we should delete it. I'd expect it to be used when walking the clk tree during rate rounding, but it's only used in the determine rate clk op. > > It's also replacing one implicit behavior by another. The point of this > series was to raise awareness on that particular point, so I'm not sure > it actually fixes things. We'll see what Stephen thinks about it. > Right. A decade ago (!) when determine_rate() was introduced we introduced CLK_SET_RATE_NO_REPARENT and set it on each mux user (see commit 819c1de344c5 ("clk: add CLK_SET_RATE_NO_REPARENT flag")). This way driver behavior wouldn't change and the status quo would be maintained, i.e. that clk_set_rate() on a mux wouldn't change parents. We didn't enforce that determine_rate exists if the set_parent() op existed at the same time though. Probably an oversight. Most of the replies to this series have been "DT is setting the parent", which makes me believe that there are 'assigned-clock-parents' being used. The clk_set_parent() path is valid for those cases. Probably nobody cares about determine_rate because they don't set rates on these clks. Some drivers even explicitly left out determine_rate()/round_rate() because they didn't want to have some other clk round up to the mux and change the parent. Eventually we want drivers to migrate to determine_rate op so we can get rid of the round_rate op and save a pointer (we're so greedy). It's been 10 years though, and that hasn't been done. Sigh! I can see value in this series from the angle of migrating, but adding a determine_rate op when there isn't a round_rate op makes it hard to reason about. What if something copies the clk_ops or sets a different flag? Now we've just added parent changing support to clk_set_rate(). What if the clk has CLK_SET_RATE_PARENT flag set? Now we're going to ask the parent clk to change rate. Fun bugs. TL;DR: If the set_parent op exists but determine_rate/round_rate doesn't then the clk is a mux that doesn't want to support clk_set_rate(). Make a new mux function that's the contents of the CLK_SET_RATE_NO_REPARENT branch in clk_mux_determine_rate_flags() and call that directly from the clk_ops so it is clear what's happening, clk_hw_mux_same_parent_determine_rate() or something with a better name. Otherwise migrate the explicit determine_rate op to this new function and don't set the flag. It may be possible to entirely remove the CLK_SET_RATE_NO_REPARENT flag with this design, if the determine_rate clk_op can call the inner wrapper function instead of __clk_mux_determine_rate*() (those underscores are awful, we should just prefix them with clk_hw_mux_*() and live happier). That should be another patch series.
Hi Stephen, On Wed, Mar 22, 2023 at 04:31:04PM -0700, Stephen Boyd wrote: > > It's also replacing one implicit behavior by another. The point of this > > series was to raise awareness on that particular point, so I'm not sure > > it actually fixes things. We'll see what Stephen thinks about it. > > > > Right. A decade ago (!) when determine_rate() was introduced we > introduced CLK_SET_RATE_NO_REPARENT and set it on each mux user (see > commit 819c1de344c5 ("clk: add CLK_SET_RATE_NO_REPARENT flag")). This > way driver behavior wouldn't change and the status quo would be > maintained, i.e. that clk_set_rate() on a mux wouldn't change parents. > We didn't enforce that determine_rate exists if the set_parent() op > existed at the same time though. Probably an oversight. > > Most of the replies to this series have been "DT is setting the parent", > which makes me believe that there are 'assigned-clock-parents' being > used. Yes, that's my understanding as well. > The clk_set_parent() path is valid for those cases. Probably nobody > cares about determine_rate because they don't set rates on these clks. > Some drivers even explicitly left out determine_rate()/round_rate() > because they didn't want to have some other clk round up to the mux > and change the parent. > > Eventually we want drivers to migrate to determine_rate op so we can get > rid of the round_rate op and save a pointer (we're so greedy). It's been > 10 years though, and that hasn't been done. Sigh! I can see value in > this series from the angle of migrating, but adding a determine_rate op > when there isn't a round_rate op makes it hard to reason about. What if > something copies the clk_ops or sets a different flag? Now we've just > added parent changing support to clk_set_rate(). What if the clk has > CLK_SET_RATE_PARENT flag set? Now we're going to ask the parent clk to > change rate. Fun bugs. > > TL;DR: If the set_parent op exists but determine_rate/round_rate doesn't > then the clk is a mux that doesn't want to support clk_set_rate(). Make > a new mux function that's the contents of the CLK_SET_RATE_NO_REPARENT > branch in clk_mux_determine_rate_flags() and call that directly from the > clk_ops so it is clear what's happening, > clk_hw_mux_same_parent_determine_rate() or something with a better name. > Otherwise migrate the explicit determine_rate op to this new function > and don't set the flag. > > It may be possible to entirely remove the CLK_SET_RATE_NO_REPARENT flag > with this design, if the determine_rate clk_op can call the inner > wrapper function instead of __clk_mux_determine_rate*() (those > underscores are awful, we should just prefix them with clk_hw_mux_*() > and live happier). That should be another patch series. Sorry but it's not really clear to me what you expect in the v2 of this series (if you even expect one). It looks that you don't like the assignment-if-missing idea Mark suggested, but should I just rebase/resend or did you expect something else? Maxime
Quoting Maxime Ripard (2023-03-29 12:50:49) > On Wed, Mar 22, 2023 at 04:31:04PM -0700, Stephen Boyd wrote: > > > The clk_set_parent() path is valid for those cases. Probably nobody > > cares about determine_rate because they don't set rates on these clks. > > Some drivers even explicitly left out determine_rate()/round_rate() > > because they didn't want to have some other clk round up to the mux > > and change the parent. > > > > Eventually we want drivers to migrate to determine_rate op so we can get > > rid of the round_rate op and save a pointer (we're so greedy). It's been > > 10 years though, and that hasn't been done. Sigh! I can see value in > > this series from the angle of migrating, but adding a determine_rate op > > when there isn't a round_rate op makes it hard to reason about. What if > > something copies the clk_ops or sets a different flag? Now we've just > > added parent changing support to clk_set_rate(). What if the clk has > > CLK_SET_RATE_PARENT flag set? Now we're going to ask the parent clk to > > change rate. Fun bugs. > > > > TL;DR: If the set_parent op exists but determine_rate/round_rate doesn't > > then the clk is a mux that doesn't want to support clk_set_rate(). Make > > a new mux function that's the contents of the CLK_SET_RATE_NO_REPARENT > > branch in clk_mux_determine_rate_flags() and call that directly from the > > clk_ops so it is clear what's happening, > > clk_hw_mux_same_parent_determine_rate() or something with a better name. > > Otherwise migrate the explicit determine_rate op to this new function > > and don't set the flag. > > > > It may be possible to entirely remove the CLK_SET_RATE_NO_REPARENT flag > > with this design, if the determine_rate clk_op can call the inner > > wrapper function instead of __clk_mux_determine_rate*() (those > > underscores are awful, we should just prefix them with clk_hw_mux_*() > > and live happier). That should be another patch series. > > Sorry but it's not really clear to me what you expect in the v2 of this > series (if you even expect one). It looks that you don't like the > assignment-if-missing idea Mark suggested, but should I just > rebase/resend or did you expect something else? > Yes, we want explicit code. Just rebase & resend. Don't add a determine_rate if there isn't a round_rate. Don't add more users of CLK_SET_RATE_NO_REPARENT. Instead, make an explicit determine_rate function for that. If you want to work on the removal of CLK_SET_RATE_NO_REPARENT go for it. Otherwise I'll take care of it after this series.
diff --git a/sound/soc/codecs/tlv320aic32x4-clk.c b/sound/soc/codecs/tlv320aic32x4-clk.c index 2f78e6820c75..65b72373cb95 100644 --- a/sound/soc/codecs/tlv320aic32x4-clk.c +++ b/sound/soc/codecs/tlv320aic32x4-clk.c @@ -41,6 +41,7 @@ struct aic32x4_clkdesc { const char * const *parent_names; unsigned int num_parents; const struct clk_ops *ops; + unsigned long flags; unsigned int reg; }; @@ -292,6 +293,7 @@ static u8 clk_aic32x4_codec_clkin_get_parent(struct clk_hw *hw) } static const struct clk_ops aic32x4_codec_clkin_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = clk_aic32x4_codec_clkin_set_parent, .get_parent = clk_aic32x4_codec_clkin_get_parent, }; @@ -401,6 +403,7 @@ static struct aic32x4_clkdesc aic32x4_clkdesc_array[] = { (const char *[]) { "mclk", "bclk", "gpio", "pll" }, .num_parents = 4, .ops = &aic32x4_codec_clkin_ops, + .flags = CLK_SET_RATE_NO_REPARENT, .reg = 0, }, { @@ -452,7 +455,7 @@ static struct clk *aic32x4_register_clk(struct device *dev, init.name = desc->name; init.parent_names = desc->parent_names; init.num_parents = desc->num_parents; - init.flags = 0; + init.flags = desc->flags; priv = devm_kzalloc(dev, sizeof(struct clk_aic32x4), GFP_KERNEL); if (priv == NULL)
The tlv320aic32x4 clkin clock implements a mux with a set_parent hook, but doesn't provide a determine_rate implementation. This is a bit odd, since set_parent() is there to, as its name implies, change the parent of a clock. However, the most likely candidate to trigger that parent change is a call to clk_set_rate(), with determine_rate() figuring out which parent is the best suited for a given rate. The other trigger would be a call to clk_set_parent(), but it's far less used, and it doesn't look like there's any obvious user for that clock. So, the set_parent hook is effectively unused, possibly because of an oversight. However, it could also be an explicit decision by the original author to avoid any reparenting but through an explicit call to clk_set_parent(). The latter case would be equivalent to setting the flag CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook to __clk_mux_determine_rate(). Indeed, if no determine_rate implementation is provided, clk_round_rate() (through clk_core_round_rate_nolock()) will call itself on the parent if CLK_SET_RATE_PARENT is set, and will not change the clock rate otherwise. __clk_mux_determine_rate() has the exact same behavior when CLK_SET_RATE_NO_REPARENT is set. And if it was an oversight, then we are at least explicit about our behavior now and it can be further refined down the line. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- sound/soc/codecs/tlv320aic32x4-clk.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)