| Message ID | 20221018-clk-range-checks-fixes-v3-43-9a1358472d52@cerno.tech |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | clk: Make determine_rate mandatory for muxes | expand |
On Tue, Apr 04, 2023 at 12:11:33PM +0200, Maxime Ripard wrote: > 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. It could be configured from device tree as well couldn't it? > 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(). Historically clk_set_rate() wouldn't reparent IIRC. > 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. To be honest it's surprising that we'd have to manually specify this, I would expect to be able to reparent. I suspect it'd be better to go the other way here and allow reparenting.
Hi Mark, On Tue, Apr 04, 2023 at 04:26:18PM +0100, Mark Brown wrote: > On Tue, Apr 04, 2023 at 12:11:33PM +0200, Maxime Ripard wrote: > > 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. > > It could be configured from device tree as well couldn't it? Yep, indeed. > > 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(). > > Historically clk_set_rate() wouldn't reparent IIRC. > > > 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. > > To be honest it's surprising that we'd have to manually specify this, I > would expect to be able to reparent. I suspect it'd be better to go the > other way here and allow reparenting. Yeah, I think I'd prefer to allow reparenting too, but as can be seen from the other reviewers in that thread, it seems like we have a very split community here, so that doesn't sound very realistic without some major pushback :) Maxime
On Wed, Apr 05, 2023 at 05:17:21PM +0200, Maxime Ripard wrote: > On Tue, Apr 04, 2023 at 04:26:18PM +0100, Mark Brown wrote: > > To be honest it's surprising that we'd have to manually specify this, I > > would expect to be able to reparent. I suspect it'd be better to go the > > other way here and allow reparenting. > Yeah, I think I'd prefer to allow reparenting too, but as can be seen > from the other reviewers in that thread, it seems like we have a very > split community here, so that doesn't sound very realistic without some > major pushback :) For these ASoC drivers I think we should just do the reparenting, they're very much at the leaf of the tree so the considerations that make it a problem sometimes are unlikely to apply.
Hi Mark, On Wed, Apr 05, 2023 at 04:34:31PM +0100, Mark Brown wrote: > On Wed, Apr 05, 2023 at 05:17:21PM +0200, Maxime Ripard wrote: > > On Tue, Apr 04, 2023 at 04:26:18PM +0100, Mark Brown wrote: > > > > To be honest it's surprising that we'd have to manually specify this, I > > > would expect to be able to reparent. I suspect it'd be better to go the > > > other way here and allow reparenting. > > > Yeah, I think I'd prefer to allow reparenting too, but as can be seen > > from the other reviewers in that thread, it seems like we have a very > > split community here, so that doesn't sound very realistic without some > > major pushback :) > > For these ASoC drivers I think we should just do the reparenting, > they're very much at the leaf of the tree so the considerations that > make it a problem sometimes are unlikely to apply. I'd still prefer to remain conservative on this series and try not to change the behaviour in it. It's pretty massive already, I'd like to avoid tracking regressions left and right :) Would sending a subsequent series that would do this acceptable for you? Maxime
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(-)