Hi,
This is a follow-up to a previous series that was printing a warning when a mux has a set_parent implementation but is missing determine_rate().
The rationale is that set_parent() is very likely to be useful when changing the rate, but it's determine_rate() that takes the parenting decision. If we're missing it, then the current parent is always going to be used, and thus set_parent() will not be used. The only exception being a direct call to clk_set_parent(), but those are fairly rare compared to clk_set_rate().
Stephen then asked to promote the warning to an error, and to fix up all the muxes that are in that situation first. So here it is :)
It was build-tested on x86, arm and arm64.
Affected drivers have been tracked down by the following coccinelle script:
virtual report
@ clk_ops @ identifier ops; position p; @@
struct clk_ops ops@p = { ... };
@ has_set_parent @ identifier clk_ops.ops; identifier set_parent_f; @@
struct clk_ops ops = { .set_parent = set_parent_f, };
@ has_determine_rate @ identifier clk_ops.ops; identifier determine_rate_f; @@
struct clk_ops ops = { .determine_rate = determine_rate_f, };
@ script:python depends on report && has_set_parent && !has_determine_rate @ ops << clk_ops.ops; set_parent_f << has_set_parent.set_parent_f; p << clk_ops.p; @@
coccilib.report.print_report(p[0], "ERROR: %s has set_parent (%s)" % (ops, set_parent_f))
Berlin is the only user still matching after this series has been applied, but it's because it uses a composite clock which throws the script off. The driver has been converted and shouldn't be a problem.
Let me know what you think, Maxime
Signed-off-by: Maxime Ripard maxime@cerno.tech --- Changes in v3: - Rebased on top of next-20230404 - Link to v2: https://lore.kernel.org/r/20221018-clk-range-checks-fixes-v2-0-f6736dec138e@...
Changes in v2: - Drop all the patches already applied - Promote the clk registration warning to an error - Make all muxes use determine_rate - Link to v1: https://lore.kernel.org/r/20221018-clk-range-checks-fixes-v1-0-f3ef80518140@...
--- Maxime Ripard (65): clk: Export clk_hw_forward_rate_request() clk: lan966x: Remove unused round_rate hook clk: nodrv: Add a determine_rate hook clk: test: Add a determine_rate hook clk: actions: composite: Add a determine_rate hook for pass clk clk: at91: main: Add a determine_rate hook clk: at91: sckc: Add a determine_rate hook clk: berlin: div: Add a determine_rate hook clk: cdce706: Add a determine_rate hook clk: k210: pll: Add a determine_rate hook clk: k210: aclk: Add a determine_rate hook clk: k210: mux: Add a determine_rate hook clk: lmk04832: clkout: Add a determine_rate hook clk: lochnagar: Add a determine_rate hook clk: qoriq: Add a determine_rate hook clk: si5341: Add a determine_rate hook clk: stm32f4: mux: Add a determine_rate hook clk: vc5: mux: Add a determine_rate hook clk: vc5: clkout: Add a determine_rate hook clk: wm831x: clkout: Add a determine_rate hook clk: davinci: da8xx-cfgchip: Add a determine_rate hook clk: davinci: da8xx-cfgchip: Add a determine_rate hook clk: imx: busy: Add a determine_rate hook clk: imx: fixup-mux: Add a determine_rate hook clk: imx: scu: Add a determine_rate hook clk: mediatek: cpumux: Add a determine_rate hook clk: pxa: Add a determine_rate hook clk: renesas: r9a06g032: Add a determine_rate hook clk: socfpga: gate: Add a determine_rate hook clk: stm32: core: Add a determine_rate hook clk: tegra: bpmp: Add a determine_rate hook clk: tegra: super: Add a determine_rate hook clk: tegra: periph: Add a determine_rate hook clk: ux500: prcmu: Add a determine_rate hook clk: ux500: sysctrl: Add a determine_rate hook clk: versatile: sp810: Add a determine_rate hook drm/tegra: sor: Add a determine_rate hook phy: cadence: sierra: Add a determine_rate hook phy: cadence: torrent: Add a determine_rate hook phy: ti: am654-serdes: Add a determine_rate hook phy: ti: j721e-wiz: Add a determine_rate hook rtc: sun6i: Add a determine_rate hook ASoC: tlv320aic32x4: Add a determine_rate hook clk: actions: composite: div: Switch to determine_rate clk: actions: composite: fact: Switch to determine_rate clk: at91: smd: Switch to determine_rate clk: axi-clkgen: Switch to determine_rate clk: cdce706: divider: Switch to determine_rate clk: cdce706: clkout: Switch to determine_rate clk: si5341: Switch to determine_rate clk: si5351: pll: Switch to determine_rate clk: si5351: msynth: Switch to determine_rate clk: si5351: clkout: Switch to determine_rate clk: da8xx: clk48: Switch to determine_rate clk: imx: scu: Switch to determine_rate clk: ingenic: cgu: Switch to determine_rate clk: ingenic: tcu: Switch to determine_rate clk: sprd: composite: Switch to determine_rate clk: st: flexgen: Switch to determine_rate clk: stm32: composite: Switch to determine_rate clk: tegra: periph: Switch to determine_rate clk: tegra: super: Switch to determine_rate ASoC: tlv320aic32x4: pll: Switch to determine_rate ASoC: tlv320aic32x4: div: Switch to determine_rate clk: Forbid to register a mux without determine_rate
drivers/clk/actions/owl-composite.c | 35 +++++++++++----- drivers/clk/actions/owl-composite.h | 2 +- drivers/clk/at91/clk-main.c | 3 +- drivers/clk/at91/clk-smd.c | 29 +++++++------ drivers/clk/at91/sckc.c | 3 +- drivers/clk/berlin/berlin2-div.c | 3 +- drivers/clk/clk-axi-clkgen.c | 14 ++++--- drivers/clk/clk-cdce706.c | 31 ++++++++------ drivers/clk/clk-k210.c | 17 +++++--- drivers/clk/clk-lan966x.c | 17 -------- drivers/clk/clk-lmk04832.c | 1 + drivers/clk/clk-lochnagar.c | 2 + drivers/clk/clk-qoriq.c | 10 +++-- drivers/clk/clk-si5341.c | 21 +++++----- drivers/clk/clk-si5351.c | 67 +++++++++++++++++-------------- drivers/clk/clk-stm32f4.c | 3 +- drivers/clk/clk-versaclock5.c | 8 ++-- drivers/clk/clk-wm831x.c | 3 +- drivers/clk/clk.c | 15 +++++++ drivers/clk/clk_test.c | 1 + drivers/clk/davinci/da8xx-cfgchip.c | 15 ++++--- drivers/clk/imx/clk-busy.c | 3 +- drivers/clk/imx/clk-fixup-mux.c | 3 +- drivers/clk/imx/clk-scu.c | 27 +++++++++++-- drivers/clk/ingenic/cgu.c | 15 +++---- drivers/clk/ingenic/tcu.c | 19 +++++---- drivers/clk/mediatek/clk-cpumux.c | 3 +- drivers/clk/pxa/clk-pxa.c | 3 +- drivers/clk/renesas/r9a06g032-clocks.c | 3 +- drivers/clk/socfpga/clk-gate.c | 3 +- drivers/clk/sprd/composite.c | 16 +++++--- drivers/clk/st/clk-flexgen.c | 15 +++---- drivers/clk/stm32/clk-stm32-core.c | 33 ++++++++++----- drivers/clk/tegra/clk-bpmp.c | 7 +++- drivers/clk/tegra/clk-periph.c | 19 ++++++--- drivers/clk/tegra/clk-super.c | 18 ++++++--- drivers/clk/ux500/clk-prcmu.c | 3 +- drivers/clk/ux500/clk-sysctrl.c | 4 +- drivers/clk/versatile/clk-sp810.c | 3 +- drivers/gpu/drm/tegra/sor.c | 3 +- drivers/phy/cadence/phy-cadence-sierra.c | 1 + drivers/phy/cadence/phy-cadence-torrent.c | 1 + drivers/phy/ti/phy-am654-serdes.c | 1 + drivers/phy/ti/phy-j721e-wiz.c | 1 + drivers/rtc/rtc-sun6i.c | 2 + sound/soc/codecs/tlv320aic32x4-clk.c | 37 ++++++++++------- 46 files changed, 343 insertions(+), 200 deletions(-) --- base-commit: 6a53bda3aaf3de5edeea27d0b1d8781d067640b6 change-id: 20221018-clk-range-checks-fixes-2039f3523240
Best regards,
Commit 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent") introduced the public clk_hw_forward_rate_request() function, but didn't export the symbol. Make sure it's the case.
Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 27c30a533759..e495dd7a1eae 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1549,6 +1549,7 @@ void clk_hw_forward_rate_request(const struct clk_hw *hw, parent->core, req, parent_rate); } +EXPORT_SYMBOL_GPL(clk_hw_forward_rate_request);
static bool clk_core_can_round(struct clk_core * const core) {
The lan966x driver registers a gck clock with both a determine_rate and a round_rate implementation. Both are equivalent, and are only called by clk_core_determine_round_nolock() which favors determine_rate.
Thus, lan966x_gck_round_rate() is never called, so we can just remove it.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-lan966x.c | 17 ----------------- 1 file changed, 17 deletions(-)
diff --git a/drivers/clk/clk-lan966x.c b/drivers/clk/clk-lan966x.c index 460e7216bfa1..870fd7df50c1 100644 --- a/drivers/clk/clk-lan966x.c +++ b/drivers/clk/clk-lan966x.c @@ -103,22 +103,6 @@ static int lan966x_gck_set_rate(struct clk_hw *hw, return 0; }
-static long lan966x_gck_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) -{ - unsigned int div; - - if (rate == 0 || *parent_rate == 0) - return -EINVAL; - - if (rate >= *parent_rate) - return *parent_rate; - - div = DIV_ROUND_CLOSEST(*parent_rate, rate); - - return *parent_rate / div; -} - static unsigned long lan966x_gck_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) { @@ -177,7 +161,6 @@ static const struct clk_ops lan966x_gck_ops = { .enable = lan966x_gck_enable, .disable = lan966x_gck_disable, .set_rate = lan966x_gck_set_rate, - .round_rate = lan966x_gck_round_rate, .recalc_rate = lan966x_gck_recalc_rate, .determine_rate = lan966x_gck_determine_rate, .set_parent = lan966x_gck_set_parent,
The nodrv clock implements a mux with a set_parent hook, but doesn't provide a determine_rate implementation.
Even though it's a mock clock and the missing function is harmless, we'll start to require a determine_rate implementation when set_parent is set, so let's fill it.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index e495dd7a1eae..f9fc8730ed17 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -4302,11 +4302,18 @@ static int clk_nodrv_set_parent(struct clk_hw *hw, u8 index) return -ENXIO; }
+static int clk_nodrv_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + return -ENXIO; +} + static const struct clk_ops clk_nodrv_ops = { .enable = clk_nodrv_prepare_enable, .disable = clk_nodrv_disable_unprepare, .prepare = clk_nodrv_prepare_enable, .unprepare = clk_nodrv_disable_unprepare, + .determine_rate = clk_nodrv_determine_rate, .set_rate = clk_nodrv_set_rate, .set_parent = clk_nodrv_set_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 | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c index f9a5c2964c65..b4f37fbd180b 100644 --- a/drivers/clk/clk_test.c +++ b/drivers/clk/clk_test.c @@ -104,6 +104,7 @@ static const struct clk_ops clk_dummy_minimize_rate_ops = { };
static const struct clk_ops clk_dummy_single_parent_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = clk_dummy_single_set_parent, .get_parent = clk_dummy_single_get_parent, };
The Actions "Pass" 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 --- drivers/clk/actions/owl-composite.c | 1 + drivers/clk/actions/owl-composite.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/actions/owl-composite.c b/drivers/clk/actions/owl-composite.c index 101706e0c66f..4c844a5613e4 100644 --- a/drivers/clk/actions/owl-composite.c +++ b/drivers/clk/actions/owl-composite.c @@ -189,6 +189,7 @@ const struct clk_ops owl_comp_fix_fact_ops = {
const struct clk_ops owl_comp_pass_ops = { /* mux_ops */ + .determine_rate = __clk_mux_determine_rate, .get_parent = owl_comp_get_parent, .set_parent = owl_comp_set_parent,
diff --git a/drivers/clk/actions/owl-composite.h b/drivers/clk/actions/owl-composite.h index bca38bf8f218..0a0eecc78344 100644 --- a/drivers/clk/actions/owl-composite.h +++ b/drivers/clk/actions/owl-composite.h @@ -104,7 +104,7 @@ struct owl_composite { .hw.init = CLK_HW_INIT_PARENTS(_name, \ _parent, \ &owl_comp_pass_ops,\ - _flags), \ + _flags | CLK_SET_RATE_NO_REPARENT), \ }, \ }
The SAM9x5 main 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 --- drivers/clk/at91/clk-main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c index 8601b27c1ae0..e04e72394632 100644 --- a/drivers/clk/at91/clk-main.c +++ b/drivers/clk/at91/clk-main.c @@ -533,6 +533,7 @@ static const struct clk_ops sam9x5_main_ops = { .prepare = clk_sam9x5_main_prepare, .is_prepared = clk_sam9x5_main_is_prepared, .recalc_rate = clk_sam9x5_main_recalc_rate, + .determine_rate = __clk_mux_determine_rate, .set_parent = clk_sam9x5_main_set_parent, .get_parent = clk_sam9x5_main_get_parent, .save_context = clk_sam9x5_main_save_context, @@ -565,7 +566,7 @@ at91_clk_register_sam9x5_main(struct regmap *regmap, init.ops = &sam9x5_main_ops; init.parent_names = parent_names; init.num_parents = num_parents; - init.flags = CLK_SET_PARENT_GATE; + init.flags = CLK_SET_PARENT_GATE | CLK_SET_RATE_NO_REPARENT;
clkmain->hw.init = &init; clkmain->regmap = regmap;
On 04.04.2023 13:10, Maxime Ripard wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
The SAM9x5 main 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
Reviewed-by: Claudiu Beznea claudiu.beznea@microchip.com
Tested-by: Claudiu Beznea claudiu.beznea@microchip.com
drivers/clk/at91/clk-main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c index 8601b27c1ae0..e04e72394632 100644 --- a/drivers/clk/at91/clk-main.c +++ b/drivers/clk/at91/clk-main.c @@ -533,6 +533,7 @@ static const struct clk_ops sam9x5_main_ops = { .prepare = clk_sam9x5_main_prepare, .is_prepared = clk_sam9x5_main_is_prepared, .recalc_rate = clk_sam9x5_main_recalc_rate,
.determine_rate = __clk_mux_determine_rate, .set_parent = clk_sam9x5_main_set_parent, .get_parent = clk_sam9x5_main_get_parent, .save_context = clk_sam9x5_main_save_context,@@ -565,7 +566,7 @@ at91_clk_register_sam9x5_main(struct regmap *regmap, init.ops = &sam9x5_main_ops; init.parent_names = parent_names; init.num_parents = num_parents;
init.flags = CLK_SET_PARENT_GATE;
init.flags = CLK_SET_PARENT_GATE | CLK_SET_RATE_NO_REPARENT; clkmain->hw.init = &init; clkmain->regmap = regmap;-- 2.39.2
The SAM9x5 slow 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 --- drivers/clk/at91/sckc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c index fdc9b669f8a7..9c42961a8a2f 100644 --- a/drivers/clk/at91/sckc.c +++ b/drivers/clk/at91/sckc.c @@ -310,6 +310,7 @@ static u8 clk_sam9x5_slow_get_parent(struct clk_hw *hw) }
static const struct clk_ops sam9x5_slow_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = clk_sam9x5_slow_set_parent, .get_parent = clk_sam9x5_slow_get_parent, }; @@ -337,7 +338,7 @@ at91_clk_register_sam9x5_slow(void __iomem *sckcr, init.ops = &sam9x5_slow_ops; init.parent_names = parent_names; init.num_parents = num_parents; - init.flags = 0; + init.flags = CLK_SET_RATE_NO_REPARENT;
slowck->hw.init = &init; slowck->sckcr = sckcr;
On 04.04.2023 13:10, Maxime Ripard wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
The SAM9x5 slow 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
Reviewed-by: Claudiu Beznea claudiu.beznea@microchip.com
Tested-by: Claudiu Beznea claudiu.beznea@microchip.com
drivers/clk/at91/sckc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c index fdc9b669f8a7..9c42961a8a2f 100644 --- a/drivers/clk/at91/sckc.c +++ b/drivers/clk/at91/sckc.c @@ -310,6 +310,7 @@ static u8 clk_sam9x5_slow_get_parent(struct clk_hw *hw) }
static const struct clk_ops sam9x5_slow_ops = {
.determine_rate = __clk_mux_determine_rate, .set_parent = clk_sam9x5_slow_set_parent, .get_parent = clk_sam9x5_slow_get_parent,}; @@ -337,7 +338,7 @@ at91_clk_register_sam9x5_slow(void __iomem *sckcr, init.ops = &sam9x5_slow_ops; init.parent_names = parent_names; init.num_parents = num_parents;
init.flags = 0;
init.flags = CLK_SET_RATE_NO_REPARENT; slowck->hw.init = &init; slowck->sckcr = sckcr;-- 2.39.2
The Berlin2 divider 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 --- drivers/clk/berlin/berlin2-div.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/berlin/berlin2-div.c b/drivers/clk/berlin/berlin2-div.c index eb14a5bc0507..d3856ec93c12 100644 --- a/drivers/clk/berlin/berlin2-div.c +++ b/drivers/clk/berlin/berlin2-div.c @@ -210,6 +210,7 @@ static unsigned long berlin2_div_recalc_rate(struct clk_hw *hw, }
static const struct clk_ops berlin2_div_rate_ops = { + .determine_rate = __clk_mux_determine_rate, .recalc_rate = berlin2_div_recalc_rate, };
@@ -251,5 +252,5 @@ berlin2_div_register(const struct berlin2_div_map *map,
return clk_hw_register_composite(NULL, name, parent_names, num_parents, &div->hw, mux_ops, &div->hw, rate_ops, - &div->hw, gate_ops, flags); + &div->hw, gate_ops, flags | CLK_SET_RATE_NO_REPARENT); }
The cdce706 "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 --- drivers/clk/clk-cdce706.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/clk/clk-cdce706.c b/drivers/clk/clk-cdce706.c index 1449d0537674..dc046bbf83a1 100644 --- a/drivers/clk/clk-cdce706.c +++ b/drivers/clk/clk-cdce706.c @@ -155,6 +155,7 @@ static u8 cdce706_clkin_get_parent(struct clk_hw *hw) }
static const struct clk_ops cdce706_clkin_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = cdce706_clkin_set_parent, .get_parent = cdce706_clkin_get_parent, }; @@ -471,6 +472,7 @@ static int cdce706_register_clkin(struct cdce706_dev_data *cdce) { struct clk_init_data init = { .ops = &cdce706_clkin_ops, + .flags = CLK_SET_RATE_NO_REPARENT, .parent_names = cdce->clkin_name, .num_parents = ARRAY_SIZE(cdce->clkin_name), };
The K210 PLL 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 --- drivers/clk/clk-k210.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/clk-k210.c b/drivers/clk/clk-k210.c index 4eed667eddaf..a96ab8611e1f 100644 --- a/drivers/clk/clk-k210.c +++ b/drivers/clk/clk-k210.c @@ -537,6 +537,7 @@ static const struct clk_ops k210_pll2_ops = { .disable = k210_pll_disable, .is_enabled = k210_pll_is_enabled, .recalc_rate = k210_pll_get_rate, + .determine_rate = __clk_mux_determine_rate, .set_parent = k210_pll2_set_parent, .get_parent = k210_pll2_get_parent, }; @@ -544,7 +545,8 @@ static const struct clk_ops k210_pll2_ops = { static int __init k210_register_pll(struct device_node *np, struct k210_sysclk *ksc, enum k210_pll_id pllid, const char *name, - int num_parents, const struct clk_ops *ops) + int num_parents, const struct clk_ops *ops, + unsigned long flags) { struct k210_pll *pll = &ksc->plls[pllid]; struct clk_init_data init = {}; @@ -558,6 +560,7 @@ static int __init k210_register_pll(struct device_node *np, init.parent_data = parent_data; init.num_parents = num_parents; init.ops = ops; + init.flags = flags;
pll->hw.init = &init; pll->ksc = ksc; @@ -574,19 +577,20 @@ static int __init k210_register_plls(struct device_node *np, k210_init_pll(ksc->regs, i, &ksc->plls[i]);
/* PLL0 and PLL1 only have IN0 as parent */ - ret = k210_register_pll(np, ksc, K210_PLL0, "pll0", 1, &k210_pll_ops); + ret = k210_register_pll(np, ksc, K210_PLL0, "pll0", 1, &k210_pll_ops, 0); if (ret) { pr_err("%pOFP: register PLL0 failed\n", np); return ret; } - ret = k210_register_pll(np, ksc, K210_PLL1, "pll1", 1, &k210_pll_ops); + ret = k210_register_pll(np, ksc, K210_PLL1, "pll1", 1, &k210_pll_ops, 0); if (ret) { pr_err("%pOFP: register PLL1 failed\n", np); return ret; }
/* PLL2 has IN0, PLL0 and PLL1 as parents */ - ret = k210_register_pll(np, ksc, K210_PLL2, "pll2", 3, &k210_pll2_ops); + ret = k210_register_pll(np, ksc, K210_PLL2, "pll2", 3, &k210_pll2_ops, + CLK_SET_RATE_NO_REPARENT); if (ret) { pr_err("%pOFP: register PLL2 failed\n", np); return ret;
The K210 ACLK 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 --- drivers/clk/clk-k210.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/clk/clk-k210.c b/drivers/clk/clk-k210.c index a96ab8611e1f..4cd6544ab102 100644 --- a/drivers/clk/clk-k210.c +++ b/drivers/clk/clk-k210.c @@ -639,6 +639,7 @@ static unsigned long k210_aclk_get_rate(struct clk_hw *hw, }
static const struct clk_ops k210_aclk_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = k210_aclk_set_parent, .get_parent = k210_aclk_get_parent, .recalc_rate = k210_aclk_get_rate, @@ -661,6 +662,7 @@ static int __init k210_register_aclk(struct device_node *np, init.parent_data = parent_data; init.num_parents = 2; init.ops = &k210_aclk_ops; + init.flags = CLK_SET_RATE_NO_REPARENT; ksc->aclk.init = &init;
ret = of_clk_hw_register(np, &ksc->aclk);
The K210 mux 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 --- drivers/clk/clk-k210.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-k210.c b/drivers/clk/clk-k210.c index 4cd6544ab102..934ed479de57 100644 --- a/drivers/clk/clk-k210.c +++ b/drivers/clk/clk-k210.c @@ -780,6 +780,7 @@ static unsigned long k210_clk_get_rate(struct clk_hw *hw, static const struct clk_ops k210_clk_mux_ops = { .enable = k210_clk_enable, .disable = k210_clk_disable, + .determine_rate = __clk_mux_determine_rate, .set_parent = k210_clk_set_parent, .get_parent = k210_clk_get_parent, .recalc_rate = k210_clk_get_rate, @@ -832,7 +833,7 @@ static inline void __init k210_register_mux_clk(struct device_node *np, { .hw = &ksc->plls[K210_PLL0].hw } };
- k210_register_clk(np, ksc, id, parent_data, 2, 0); + k210_register_clk(np, ksc, id, parent_data, 2, CLK_SET_RATE_NO_REPARENT); }
static inline void __init k210_register_in0_child(struct device_node *np,
The LKM04832 "CLKOUT" 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.
Since the CLK_SET_RATE_NO_REPARENT flag was already set though, it seems unlikely.
Reviewed-by: Liam Beguin liambeguin@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-lmk04832.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/clk/clk-lmk04832.c b/drivers/clk/clk-lmk04832.c index 57485356de4c..f7fbfb09c7af 100644 --- a/drivers/clk/clk-lmk04832.c +++ b/drivers/clk/clk-lmk04832.c @@ -1279,6 +1279,7 @@ static const struct clk_ops lmk04832_clkout_ops = { .is_enabled = lmk04832_clkout_is_enabled, .prepare = lmk04832_clkout_prepare, .unprepare = lmk04832_clkout_unprepare, + .determine_rate = __clk_mux_determine_rate, .set_parent = lmk04832_clkout_set_parent, .get_parent = lmk04832_clkout_get_parent, };
The lochnagar clocks implement 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 --- drivers/clk/clk-lochnagar.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/clk/clk-lochnagar.c b/drivers/clk/clk-lochnagar.c index 80944bf482e9..820c05732ac7 100644 --- a/drivers/clk/clk-lochnagar.c +++ b/drivers/clk/clk-lochnagar.c @@ -209,6 +209,7 @@ static u8 lochnagar_clk_get_parent(struct clk_hw *hw) static const struct clk_ops lochnagar_clk_ops = { .prepare = lochnagar_clk_prepare, .unprepare = lochnagar_clk_unprepare, + .determine_rate = __clk_mux_determine_rate, .set_parent = lochnagar_clk_set_parent, .get_parent = lochnagar_clk_get_parent, }; @@ -238,6 +239,7 @@ static int lochnagar_clk_probe(struct platform_device *pdev) { struct clk_init_data clk_init = { .ops = &lochnagar_clk_ops, + .flags = CLK_SET_RATE_NO_REPARENT, }; struct device *dev = &pdev->dev; struct lochnagar_clk_priv *priv;
On Tue, Apr 04, 2023 at 12:11:04PM +0200, Maxime Ripard wrote:
The lochnagar clocks implement 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
Tested-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
The Qoriq mux clocks implement 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 --- drivers/clk/clk-qoriq.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c index 5eddb9f0d6bd..6f51a2cfaace 100644 --- a/drivers/clk/clk-qoriq.c +++ b/drivers/clk/clk-qoriq.c @@ -878,6 +878,7 @@ static u8 mux_get_parent(struct clk_hw *hw) }
static const struct clk_ops cmux_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = mux_get_parent, .set_parent = mux_set_parent, }; @@ -908,6 +909,7 @@ static const struct clockgen_pll_div *get_pll_div(struct clockgen *cg, static struct clk * __init create_mux_common(struct clockgen *cg, struct mux_hwclock *hwc, const struct clk_ops *ops, + unsigned long flags, unsigned long min_rate, unsigned long max_rate, unsigned long pct80_rate, @@ -951,7 +953,7 @@ static struct clk * __init create_mux_common(struct clockgen *cg, init.ops = ops; init.parent_names = parent_names; init.num_parents = hwc->num_parents = j; - init.flags = 0; + init.flags = flags; hwc->hw.init = &init; hwc->cg = cg;
@@ -1010,8 +1012,8 @@ static struct clk * __init create_one_cmux(struct clockgen *cg, int idx) else min_rate = plat_rate / 2;
- return create_mux_common(cg, hwc, &cmux_ops, min_rate, max_rate, - pct80_rate, "cg-cmux%d", idx); + return create_mux_common(cg, hwc, &cmux_ops, CLK_SET_RATE_NO_REPARENT, + min_rate, max_rate, pct80_rate, "cg-cmux%d", idx); }
static struct clk * __init create_one_hwaccel(struct clockgen *cg, int idx) @@ -1025,7 +1027,7 @@ static struct clk * __init create_one_hwaccel(struct clockgen *cg, int idx) hwc->reg = cg->regs + 0x20 * idx + 0x10; hwc->info = cg->info.hwaccel[idx];
- return create_mux_common(cg, hwc, &hwaccel_ops, 0, ULONG_MAX, 0, + return create_mux_common(cg, hwc, &hwaccel_ops, 0, 0, ULONG_MAX, 0, "cg-hwaccel%d", idx); }
The SI5341 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 --- drivers/clk/clk-si5341.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c index 0e528d7ba656..259861aa2e2f 100644 --- a/drivers/clk/clk-si5341.c +++ b/drivers/clk/clk-si5341.c @@ -551,6 +551,7 @@ static int si5341_clk_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops si5341_clk_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = si5341_clk_set_parent, .get_parent = si5341_clk_get_parent, .recalc_rate = si5341_clk_recalc_rate, @@ -1682,7 +1683,7 @@ static int si5341_probe(struct i2c_client *client) init.parent_names = data->input_clk_name; init.num_parents = SI5341_NUM_INPUTS; init.ops = &si5341_clk_ops; - init.flags = 0; + init.flags = CLK_SET_RATE_NO_REPARENT; data->hw.init = &init;
err = devm_clk_hw_register(&client->dev, &data->hw);
The STM32F4 mux 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 --- drivers/clk/clk-stm32f4.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c index 473dfe632cc5..01046437a48c 100644 --- a/drivers/clk/clk-stm32f4.c +++ b/drivers/clk/clk-stm32f4.c @@ -1045,6 +1045,7 @@ static int cclk_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops cclk_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = cclk_mux_get_parent, .set_parent = cclk_mux_set_parent, }; @@ -1085,7 +1086,7 @@ static struct clk_hw *stm32_register_cclk(struct device *dev, const char *name, &mux->hw, &cclk_mux_ops, NULL, NULL, &gate->hw, &cclk_gate_ops, - flags); + flags | CLK_SET_RATE_NO_REPARENT);
if (IS_ERR(hw)) { kfree(gate);
The Versaclock5 mux 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 --- drivers/clk/clk-versaclock5.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c index fa71a57875ce..4b68d919f75b 100644 --- a/drivers/clk/clk-versaclock5.c +++ b/drivers/clk/clk-versaclock5.c @@ -281,6 +281,7 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops vc5_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = vc5_mux_set_parent, .get_parent = vc5_mux_get_parent, }; @@ -1029,7 +1030,7 @@ static int vc5_probe(struct i2c_client *client)
init.name = kasprintf(GFP_KERNEL, "%pOFn.mux", client->dev.of_node); init.ops = &vc5_mux_ops; - init.flags = 0; + init.flags = CLK_SET_RATE_NO_REPARENT; init.parent_names = parent_names; vc5->clk_mux.init = &init; ret = devm_clk_hw_register(&client->dev, &vc5->clk_mux);
The Versaclock5 "clkout" 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 --- drivers/clk/clk-versaclock5.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c index 4b68d919f75b..71e955429234 100644 --- a/drivers/clk/clk-versaclock5.c +++ b/drivers/clk/clk-versaclock5.c @@ -726,6 +726,7 @@ static int vc5_clk_out_set_parent(struct clk_hw *hw, u8 index) static const struct clk_ops vc5_clk_out_ops = { .prepare = vc5_clk_out_prepare, .unprepare = vc5_clk_out_unprepare, + .determine_rate = __clk_mux_determine_rate, .set_parent = vc5_clk_out_set_parent, .get_parent = vc5_clk_out_get_parent, }; @@ -1113,7 +1114,7 @@ static int vc5_probe(struct i2c_client *client) init.name = kasprintf(GFP_KERNEL, "%pOFn.out0_sel_i2cb", client->dev.of_node); init.ops = &vc5_clk_out_ops; - init.flags = CLK_SET_RATE_PARENT; + init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; init.parent_names = parent_names; parent_names[0] = clk_hw_get_name(&vc5->clk_mux); init.num_parents = 1; @@ -1139,7 +1140,7 @@ static int vc5_probe(struct i2c_client *client) init.name = kasprintf(GFP_KERNEL, "%pOFn.out%d", client->dev.of_node, idx + 1); init.ops = &vc5_clk_out_ops; - init.flags = CLK_SET_RATE_PARENT; + init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; init.parent_names = parent_names; init.num_parents = 2; vc5->clk_out[n].num = idx;
The WM381x "clkout" 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.
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-wm831x.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c index ae6dd38ec053..be3a9c1f3610 100644 --- a/drivers/clk/clk-wm831x.c +++ b/drivers/clk/clk-wm831x.c @@ -329,6 +329,7 @@ static const struct clk_ops wm831x_clkout_ops = { .is_prepared = wm831x_clkout_is_prepared, .prepare = wm831x_clkout_prepare, .unprepare = wm831x_clkout_unprepare, + .determine_rate = __clk_mux_determine_rate, .get_parent = wm831x_clkout_get_parent, .set_parent = wm831x_clkout_set_parent, }; @@ -338,7 +339,7 @@ static const struct clk_init_data wm831x_clkout_init = { .ops = &wm831x_clkout_ops, .parent_names = wm831x_clkout_parents, .num_parents = ARRAY_SIZE(wm831x_clkout_parents), - .flags = CLK_SET_RATE_PARENT, + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT, };
static int wm831x_clk_probe(struct platform_device *pdev)
The Davinci DA8xxx cfgchip mux 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.
Acked-by: David Lechner david@lechnology.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/davinci/da8xx-cfgchip.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c index 4103d605e804..c04276bc4051 100644 --- a/drivers/clk/davinci/da8xx-cfgchip.c +++ b/drivers/clk/davinci/da8xx-cfgchip.c @@ -229,6 +229,7 @@ static u8 da8xx_cfgchip_mux_clk_get_parent(struct clk_hw *hw) }
static const struct clk_ops da8xx_cfgchip_mux_clk_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = da8xx_cfgchip_mux_clk_set_parent, .get_parent = da8xx_cfgchip_mux_clk_get_parent, }; @@ -251,7 +252,7 @@ da8xx_cfgchip_mux_clk_register(struct device *dev, init.ops = &da8xx_cfgchip_mux_clk_ops; init.parent_names = parent_names; init.num_parents = 2; - init.flags = 0; + init.flags = CLK_SET_RATE_NO_REPARENT;
mux->hw.init = &init; mux->regmap = regmap;
On 4/4/23 5:11 AM, Maxime Ripard wrote:
The Davinci DA8xxx cfgchip mux 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.
As mentioned in my previous review, parent is selected by device tree and should never be changed after init.
The Davinci DA8xxx cfgchip "clk48" 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 --- drivers/clk/davinci/da8xx-cfgchip.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c index c04276bc4051..4c1cc59bba53 100644 --- a/drivers/clk/davinci/da8xx-cfgchip.c +++ b/drivers/clk/davinci/da8xx-cfgchip.c @@ -565,6 +565,7 @@ static u8 da8xx_usb1_clk48_get_parent(struct clk_hw *hw) }
static const struct clk_ops da8xx_usb1_clk48_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = da8xx_usb1_clk48_set_parent, .get_parent = da8xx_usb1_clk48_get_parent, }; @@ -589,6 +590,7 @@ da8xx_cfgchip_register_usb1_clk48(struct device *dev,
init.name = "usb1_clk48"; init.ops = &da8xx_usb1_clk48_ops; + init.flags = CLK_SET_RATE_NO_REPARENT; init.parent_names = parent_names; init.num_parents = 2;
On 4/4/23 5:11 AM, Maxime Ripard wrote:
The Davinci DA8xxx cfgchip "clk48" 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.
As mentioned in my previous review, parent is selected by device tree and should never be changed after init.
The iMX busy 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 --- drivers/clk/imx/clk-busy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/imx/clk-busy.c b/drivers/clk/imx/clk-busy.c index 6f17311647f3..2df81862782a 100644 --- a/drivers/clk/imx/clk-busy.c +++ b/drivers/clk/imx/clk-busy.c @@ -148,6 +148,7 @@ static int clk_busy_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops clk_busy_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = clk_busy_mux_get_parent, .set_parent = clk_busy_mux_set_parent, }; @@ -176,7 +177,7 @@ struct clk_hw *imx_clk_hw_busy_mux(const char *name, void __iomem *reg, u8 shift
init.name = name; init.ops = &clk_busy_mux_ops; - init.flags = CLK_IS_CRITICAL; + init.flags = CLK_IS_CRITICAL | CLK_SET_RATE_NO_REPARENT; init.parent_names = parent_names; init.num_parents = num_parents;
The iMX fixup mux 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 --- drivers/clk/imx/clk-fixup-mux.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/imx/clk-fixup-mux.c b/drivers/clk/imx/clk-fixup-mux.c index c82401570c84..e32c3b22ff05 100644 --- a/drivers/clk/imx/clk-fixup-mux.c +++ b/drivers/clk/imx/clk-fixup-mux.c @@ -60,6 +60,7 @@ static int clk_fixup_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops clk_fixup_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = clk_fixup_mux_get_parent, .set_parent = clk_fixup_mux_set_parent, }; @@ -84,7 +85,7 @@ struct clk_hw *imx_clk_hw_fixup_mux(const char *name, void __iomem *reg, init.ops = &clk_fixup_mux_ops; init.parent_names = parents; init.num_parents = num_parents; - init.flags = 0; + init.flags = CLK_SET_RATE_NO_REPARENT;
fixup_mux->mux.reg = reg; fixup_mux->mux.shift = shift;
The iMX SCU mux 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 --- drivers/clk/imx/clk-scu.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c index 1e6870f3671f..66e49fea5f8a 100644 --- a/drivers/clk/imx/clk-scu.c +++ b/drivers/clk/imx/clk-scu.c @@ -785,6 +785,7 @@ static int clk_gpr_mux_scu_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops clk_gpr_mux_scu_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = clk_gpr_mux_scu_get_parent, .set_parent = clk_gpr_mux_scu_set_parent, }; @@ -836,7 +837,7 @@ struct clk_hw *__imx_clk_gpr_scu(const char *name, const char * const *parent_na struct imx_scu_clk_node *clk_node; struct clk_gpr_scu *clk; struct clk_hw *hw; - struct clk_init_data init; + struct clk_init_data init = {}; int ret;
if (rsrc_id >= IMX_SC_R_LAST || gpr_id >= IMX_SC_C_LAST) @@ -868,10 +869,11 @@ struct clk_hw *__imx_clk_gpr_scu(const char *name, const char * const *parent_na if (flags & IMX_SCU_GPR_CLK_DIV) init.ops = &clk_gpr_div_scu_ops;
- if (flags & IMX_SCU_GPR_CLK_MUX) + if (flags & IMX_SCU_GPR_CLK_MUX) { init.ops = &clk_gpr_mux_scu_ops; + init.flags |= CLK_SET_RATE_NO_REPARENT; + }
- init.flags = 0; init.name = name; init.parent_names = parent_name; init.num_parents = num_parents;
The Mediatek cpumux 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 --- drivers/clk/mediatek/clk-cpumux.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/mediatek/clk-cpumux.c b/drivers/clk/mediatek/clk-cpumux.c index da05f06192c0..dd4eb3f215cc 100644 --- a/drivers/clk/mediatek/clk-cpumux.c +++ b/drivers/clk/mediatek/clk-cpumux.c @@ -53,6 +53,7 @@ static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops clk_cpumux_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = clk_cpumux_get_parent, .set_parent = clk_cpumux_set_parent, }; @@ -73,7 +74,7 @@ mtk_clk_register_cpumux(struct device *dev, const struct mtk_composite *mux, init.ops = &clk_cpumux_ops; init.parent_names = mux->parent_names; init.num_parents = mux->num_parents; - init.flags = mux->flags; + init.flags = mux->flags | CLK_SET_RATE_NO_REPARENT;
cpumux->reg = mux->mux_reg; cpumux->shift = mux->mux_shift;
The PXA "CKEN" 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.
Since the set_parent implementation is a nop though, it seems unlikely.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/pxa/clk-pxa.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/pxa/clk-pxa.c b/drivers/clk/pxa/clk-pxa.c index 374098ebbf2b..47bc60c2854c 100644 --- a/drivers/clk/pxa/clk-pxa.c +++ b/drivers/clk/pxa/clk-pxa.c @@ -82,6 +82,7 @@ static u8 cken_get_parent(struct clk_hw *hw) }
static const struct clk_ops cken_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = cken_get_parent, .set_parent = dummy_clk_set_parent, }; @@ -117,7 +118,7 @@ int __init clk_pxa_cken_init(const struct desc_clk_cken *clks, &pxa_clk->hw, &cken_mux_ops, &pxa_clk->hw, &cken_rate_ops, &pxa_clk->gate.hw, &clk_gate_ops, - clks[i].flags); + clks[i].flags | CLK_SET_RATE_NO_REPARENT); clkdev_pxa_register(clks[i].ckid, clks[i].con_id, clks[i].dev_id, clk); }
The Renesas r9a06g032 bitselect 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 --- drivers/clk/renesas/r9a06g032-clocks.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c index 40828616f723..56108b37f94e 100644 --- a/drivers/clk/renesas/r9a06g032-clocks.c +++ b/drivers/clk/renesas/r9a06g032-clocks.c @@ -1121,6 +1121,7 @@ static int r9a06g032_clk_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops clk_bitselect_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = r9a06g032_clk_mux_get_parent, .set_parent = r9a06g032_clk_mux_set_parent, }; @@ -1145,7 +1146,7 @@ r9a06g032_register_bitsel(struct r9a06g032_priv *clocks,
init.name = desc->name; init.ops = &clk_bitselect_ops; - init.flags = CLK_SET_RATE_PARENT; + init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; init.parent_names = names; init.num_parents = 2;
CC Gareth, Hervé, Miquel, Ralph
On Tue, Apr 4, 2023 at 2:44 PM Maxime Ripard maxime@cerno.tech wrote:
The Renesas r9a06g032 bitselect 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
LGTM, so Reviewed-by: Geert Uytterhoeven geert+renesas@glider.be But I do not have the hardware.
--- a/drivers/clk/renesas/r9a06g032-clocks.c +++ b/drivers/clk/renesas/r9a06g032-clocks.c @@ -1121,6 +1121,7 @@ static int r9a06g032_clk_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops clk_bitselect_ops = {
.determine_rate = __clk_mux_determine_rate, .get_parent = r9a06g032_clk_mux_get_parent, .set_parent = r9a06g032_clk_mux_set_parent,}; @@ -1145,7 +1146,7 @@ r9a06g032_register_bitsel(struct r9a06g032_priv *clocks,
init.name = desc->name; init.ops = &clk_bitselect_ops;
init.flags = CLK_SET_RATE_PARENT;
init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; init.parent_names = names; init.num_parents = 2;
Gr{oetje,eeting}s,
Geert
Hi Geert & Maxime,
geert@linux-m68k.org wrote on Tue, 11 Apr 2023 12:27:38 +0200:
CC Gareth, Hervé, Miquel, Ralph
On Tue, Apr 4, 2023 at 2:44 PM Maxime Ripard maxime@cerno.tech wrote:
The Renesas r9a06g032 bitselect 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
LGTM, so Reviewed-by: Geert Uytterhoeven geert+renesas@glider.be
I searched for 'possible callers', I didn't find any places where this would be used on the consumer side. However, downstream, there is a rzn1-clock-bitselect.c clock driver which states:
+ * This clock provider handles the case of the RZN1 where you have peripherals + * that have two potential clock source and two gates, one for each of the + * clock source - the used clock source (for all sub clocks) is selected by a + * single bit. + * That single bit affects all sub-clocks, and therefore needs to change the + * active gate (and turn the others off) and force a recalculation of the rates.
I don't know how much of this file has been upstreamed (under a different form) but this might very well be related to the fact that reparenting in some cases would be a major issue and thus needs to be avoided unless done on purpose (guessing?).
Maybe Ralph can comment, but for what I understand,
Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com
But I do not have the hardware.
--- a/drivers/clk/renesas/r9a06g032-clocks.c +++ b/drivers/clk/renesas/r9a06g032-clocks.c @@ -1121,6 +1121,7 @@ static int r9a06g032_clk_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops clk_bitselect_ops = {
.determine_rate = __clk_mux_determine_rate, .get_parent = r9a06g032_clk_mux_get_parent, .set_parent = r9a06g032_clk_mux_set_parent,}; @@ -1145,7 +1146,7 @@ r9a06g032_register_bitsel(struct r9a06g032_priv *clocks,
init.name = desc->name; init.ops = &clk_bitselect_ops;
init.flags = CLK_SET_RATE_PARENT;
init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; init.parent_names = names; init.num_parents = 2;Gr{oetje,eeting}s,
Geert
Thanks, Miquèl
The SoCFGPA gate 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 --- drivers/clk/socfpga/clk-gate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c index 32ccda960f28..cbba8462a09e 100644 --- a/drivers/clk/socfpga/clk-gate.c +++ b/drivers/clk/socfpga/clk-gate.c @@ -110,6 +110,7 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk,
static struct clk_ops gateclk_ops = { .recalc_rate = socfpga_clk_recalc_rate, + .determine_rate = __clk_mux_determine_rate, .get_parent = socfpga_clk_get_parent, .set_parent = socfpga_clk_set_parent, }; @@ -166,7 +167,7 @@ void __init socfpga_gate_init(struct device_node *node)
init.name = clk_name; init.ops = ops; - init.flags = 0; + init.flags = CLK_SET_RATE_NO_REPARENT;
init.num_parents = of_clk_parent_fill(node, parent_name, SOCFPGA_MAX_PARENTS); if (init.num_parents < 2) {
Hi Maxime,
On 4/4/23 05:11, Maxime Ripard wrote:
The SoCFGPA gate 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
drivers/clk/socfpga/clk-gate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c index 32ccda960f28..cbba8462a09e 100644 --- a/drivers/clk/socfpga/clk-gate.c +++ b/drivers/clk/socfpga/clk-gate.c @@ -110,6 +110,7 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk,
static struct clk_ops gateclk_ops = { .recalc_rate = socfpga_clk_recalc_rate,
- .determine_rate = __clk_mux_determine_rate, .get_parent = socfpga_clk_get_parent, .set_parent = socfpga_clk_set_parent, };
@@ -166,7 +167,7 @@ void __init socfpga_gate_init(struct device_node *node)
init.name = clk_name; init.ops = ops;
- init.flags = 0;
init.flags = CLK_SET_RATE_NO_REPARENT;
init.num_parents = of_clk_parent_fill(node, parent_name, SOCFPGA_MAX_PARENTS); if (init.num_parents < 2) {
This patch broke SoCFPGA boot serial port. The characters are mangled.
Dinh
Hi Dinh,
On Mon, Apr 24, 2023 at 01:32:28PM -0500, Dinh Nguyen wrote:
On 4/4/23 05:11, Maxime Ripard wrote:
The SoCFGPA gate 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
drivers/clk/socfpga/clk-gate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c index 32ccda960f28..cbba8462a09e 100644 --- a/drivers/clk/socfpga/clk-gate.c +++ b/drivers/clk/socfpga/clk-gate.c @@ -110,6 +110,7 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk, static struct clk_ops gateclk_ops = { .recalc_rate = socfpga_clk_recalc_rate,
- .determine_rate = __clk_mux_determine_rate, .get_parent = socfpga_clk_get_parent, .set_parent = socfpga_clk_set_parent, };
@@ -166,7 +167,7 @@ void __init socfpga_gate_init(struct device_node *node) init.name = clk_name; init.ops = ops;
- init.flags = 0;
- init.flags = CLK_SET_RATE_NO_REPARENT; init.num_parents = of_clk_parent_fill(node, parent_name, SOCFPGA_MAX_PARENTS); if (init.num_parents < 2) {
This patch broke SoCFPGA boot serial port. The characters are mangled.
Do you have any other access to that board? If so, could you dump clk_summary in debugfs with and without that patch?
Maxime
Hi Maxime,
On 4/25/23 09:48, Maxime Ripard wrote:
Hi Dinh,
On Mon, Apr 24, 2023 at 01:32:28PM -0500, Dinh Nguyen wrote:
On 4/4/23 05:11, Maxime Ripard wrote:
The SoCFGPA gate 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
drivers/clk/socfpga/clk-gate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c index 32ccda960f28..cbba8462a09e 100644 --- a/drivers/clk/socfpga/clk-gate.c +++ b/drivers/clk/socfpga/clk-gate.c @@ -110,6 +110,7 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk, static struct clk_ops gateclk_ops = { .recalc_rate = socfpga_clk_recalc_rate,
- .determine_rate = __clk_mux_determine_rate, .get_parent = socfpga_clk_get_parent, .set_parent = socfpga_clk_set_parent, };
@@ -166,7 +167,7 @@ void __init socfpga_gate_init(struct device_node *node) init.name = clk_name; init.ops = ops;
- init.flags = 0;
- init.flags = CLK_SET_RATE_NO_REPARENT; init.num_parents = of_clk_parent_fill(node, parent_name, SOCFPGA_MAX_PARENTS); if (init.num_parents < 2) {
This patch broke SoCFPGA boot serial port. The characters are mangled.
Do you have any other access to that board? If so, could you dump clk_summary in debugfs with and without that patch?
That dump from the clk_summary are identical for both cases.
Hi Dinh,
On Thu, Apr 27, 2023 at 02:09:48PM -0500, Dinh Nguyen wrote:
Hi Maxime,
On 4/25/23 09:48, Maxime Ripard wrote:
Hi Dinh,
On Mon, Apr 24, 2023 at 01:32:28PM -0500, Dinh Nguyen wrote:
On 4/4/23 05:11, Maxime Ripard wrote:
The SoCFGPA gate 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
drivers/clk/socfpga/clk-gate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c index 32ccda960f28..cbba8462a09e 100644 --- a/drivers/clk/socfpga/clk-gate.c +++ b/drivers/clk/socfpga/clk-gate.c @@ -110,6 +110,7 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk, static struct clk_ops gateclk_ops = { .recalc_rate = socfpga_clk_recalc_rate,
- .determine_rate = __clk_mux_determine_rate, .get_parent = socfpga_clk_get_parent, .set_parent = socfpga_clk_set_parent, };
@@ -166,7 +167,7 @@ void __init socfpga_gate_init(struct device_node *node) init.name = clk_name; init.ops = ops;
- init.flags = 0;
- init.flags = CLK_SET_RATE_NO_REPARENT; init.num_parents = of_clk_parent_fill(node, parent_name, SOCFPGA_MAX_PARENTS); if (init.num_parents < 2) {
This patch broke SoCFPGA boot serial port. The characters are mangled.
Do you have any other access to that board? If so, could you dump clk_summary in debugfs with and without that patch?
That dump from the clk_summary are identical for both cases.
Thanks for testing
I'm a bit confused, there should be no difference in behaviour, and if there was any difference I would expect the clock tree to be somewhat different.
Could you still paste the clk_summary (and dmesg) output? Which UART driver is being used?
Also, is there a way for me to test it somehow?
Thanks, Maxime
Hi Maxime,
On 5/4/23 12:04, Maxime Ripard wrote:
Hi Dinh,
On Thu, Apr 27, 2023 at 02:09:48PM -0500, Dinh Nguyen wrote:
Hi Maxime,
On 4/25/23 09:48, Maxime Ripard wrote:
Hi Dinh,
On Mon, Apr 24, 2023 at 01:32:28PM -0500, Dinh Nguyen wrote:
On 4/4/23 05:11, Maxime Ripard wrote:
The SoCFGPA gate 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
drivers/clk/socfpga/clk-gate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c index 32ccda960f28..cbba8462a09e 100644 --- a/drivers/clk/socfpga/clk-gate.c +++ b/drivers/clk/socfpga/clk-gate.c @@ -110,6 +110,7 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk, static struct clk_ops gateclk_ops = { .recalc_rate = socfpga_clk_recalc_rate,
- .determine_rate = __clk_mux_determine_rate, .get_parent = socfpga_clk_get_parent, .set_parent = socfpga_clk_set_parent, };
@@ -166,7 +167,7 @@ void __init socfpga_gate_init(struct device_node *node) init.name = clk_name; init.ops = ops;
- init.flags = 0;
- init.flags = CLK_SET_RATE_NO_REPARENT; init.num_parents = of_clk_parent_fill(node, parent_name, SOCFPGA_MAX_PARENTS); if (init.num_parents < 2) {
This patch broke SoCFPGA boot serial port. The characters are mangled.
Do you have any other access to that board? If so, could you dump clk_summary in debugfs with and without that patch?
That dump from the clk_summary are identical for both cases.
Thanks for testing
I'm a bit confused, there should be no difference in behaviour, and if there was any difference I would expect the clock tree to be somewhat different.
Could you still paste the clk_summary (and dmesg) output? Which UART driver is being used?
Also, is there a way for me to test it somehow?
Apologies, but there is a diff with/without this patch:
With patch: < l4_sp_clk 3 3 0 100000000 0 0 50000 ? --- Without patch:
l4_sp_clk 4 4 0 100000000
0 0 50000 ?
The enable/prepare count is 4 instead of 3 in the case of a working UART. The debug uart is using the lp_sp_clk.
The Cyclone5 devkits are pretty cheap if you want to get one.
Here is the full out of clk_summary:
# cat /sys/kernel/debug/clk/clk_summary enable prepare protect duty hardware clock count count count rate accuracy phase cycle enable ------------------------------------------------------------------------------------------------------- osc1 5 5 0 25000000 0 0 50000 Y sdram_pll 0 0 0 800000000 0 0 50000 Y h2f_usr2_clk 0 0 0 133333333 0 0 50000 Y h2f_user2_clk 0 0 0 133333333 0 0 50000 ? ddr_dq_clk 0 0 0 400000000 0 0 50000 Y ddr_dq_clk_gate 0 0 0 400000000 0 0 50000 ? ddr_2x_dqs_clk 0 0 0 800000000 0 0 50000 Y ddr_2x_dqs_clk_gate 0 0 0 800000000 0 0 50000 ? ddr_dqs_clk 0 0 0 400000000 0 0 50000 Y ddr_dqs_clk_gate 0 0 0 400000000 0 0 50000 ? periph_pll 3 3 0 1000000000 0 0 50000 Y h2f_usr1_clk 0 0 0 1953125 0 0 50000 Y h2f_user1_clk 0 0 0 1953125 0 0 50000 ? per_base_clk 4 4 0 200000000 0 0 50000 Y gpio_db_clk 0 0 0 32000 0 0 50000 ? can1_clk 0 0 0 40000000 0 0 50000 ? can0_clk 0 0 0 100000000 0 0 50000 ? spi_m_clk 1 1 0 200000000 0 0 50000 ? usb_mp_clk 1 1 0 200000000 0 0 50000 ? l4_sp_clk 4 4 0 100000000 0 0 50000 ? l4_mp_clk 1 1 0 100000000 0 0 50000 ? per_nand_mmc_clk 1 1 0 200000000 0 0 50000 Y nand_x_clk 0 0 0 200000000 0 0 50000 ? nand_clk 0 0 0 50000000 0 0 50000 ? nand_ecc_clk 0 0 0 200000000 0 0 50000 ? sdmmc_clk 1 1 0 200000000 0 0 50000 ? sdmmc_clk_divided 1 1 0 50000000 0 0 50000 ? per_qsi_clk 0 0 0 1953125 0 0 50000 Y emac1_clk 1 1 0 250000000 0 0 50000 Y emac_1_clk 1 1 0 250000000 0 0 50000 ? emac0_clk 0 0 0 1953125 0 0 50000 Y emac_0_clk 0 0 0 1953125 0 0 50000 ? dbg_base_clk 0 0 0 6250000 0 0 50000 Y dbg_timer_clk 0 0 0 6250000 0 0 50000 ? dbg_trace_clk 0 0 0 6250000 0 0 50000 ? dbg_at_clk 0 0 0 6250000 0 0 50000 ? dbg_clk 0 0 0 3125000 0 0 50000 ? main_pll 2 3 0 1850000000 0 0 50000 Y cfg_h2f_usr0_clk 0 0 0 123333333 0 0 50000 Y h2f_user0_clk 0 0 0 123333333 0 0 50000 ? cfg_clk 0 0 0 123333333 0 0 50000 ? main_nand_sdmmc_clk 0 0 0 3613281 0 0 50000 Y main_qspi_clk 1 1 0 370000000 0 0 50000 Y qspi_clk 1 1 0 370000000 0 0 50000 ? mainclk 0 1 0 370000000 0 0 50000 Y l3_mp_clk 0 0 0 185000000 0 0 50000 ? l3_sp_clk 0 0 0 92500000 0 0 50000 Y l3_main_clk 0 0 0 370000000 0 0 50000 Y l4_main_clk 0 1 0 370000000 0 0 50000 ? mpuclk 1 1 0 925000000 0 0 50000 Y mpu_l2_ram_clk 0 0 0 462500000 0 0 50000 Y mpu_periph_clk 1 1 0 231250000 0 0 50000 Y
Dinh
Hi Dinh,
On Tue, May 09, 2023 at 12:37:39PM -0500, Dinh Nguyen wrote:
Hi Maxime,
On 5/4/23 12:04, Maxime Ripard wrote:
Hi Dinh,
On Thu, Apr 27, 2023 at 02:09:48PM -0500, Dinh Nguyen wrote:
Hi Maxime,
On 4/25/23 09:48, Maxime Ripard wrote:
Hi Dinh,
On Mon, Apr 24, 2023 at 01:32:28PM -0500, Dinh Nguyen wrote:
On 4/4/23 05:11, Maxime Ripard wrote:
The SoCFGPA gate 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
drivers/clk/socfpga/clk-gate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c index 32ccda960f28..cbba8462a09e 100644 --- a/drivers/clk/socfpga/clk-gate.c +++ b/drivers/clk/socfpga/clk-gate.c @@ -110,6 +110,7 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk, static struct clk_ops gateclk_ops = { .recalc_rate = socfpga_clk_recalc_rate,
- .determine_rate = __clk_mux_determine_rate, .get_parent = socfpga_clk_get_parent, .set_parent = socfpga_clk_set_parent, };
@@ -166,7 +167,7 @@ void __init socfpga_gate_init(struct device_node *node) init.name = clk_name; init.ops = ops;
- init.flags = 0;
- init.flags = CLK_SET_RATE_NO_REPARENT; init.num_parents = of_clk_parent_fill(node, parent_name, SOCFPGA_MAX_PARENTS); if (init.num_parents < 2) {
This patch broke SoCFPGA boot serial port. The characters are mangled.
Do you have any other access to that board? If so, could you dump clk_summary in debugfs with and without that patch?
That dump from the clk_summary are identical for both cases.
Thanks for testing
I'm a bit confused, there should be no difference in behaviour, and if there was any difference I would expect the clock tree to be somewhat different.
Could you still paste the clk_summary (and dmesg) output? Which UART driver is being used?
Also, is there a way for me to test it somehow?
Apologies, but there is a diff with/without this patch:
With patch: < l4_sp_clk 3 3 0 100000000 0 0 50000 ?
Without patch:
l4_sp_clk 4 4 0 1000000000 0 50000 ?
The enable/prepare count is 4 instead of 3 in the case of a working UART. The debug uart is using the lp_sp_clk.
This is pretty weird, the enable count shouldn't change, really, we're only changing something in the rate rounding path... Is it using the snps,dw-apb-uart driver?
Nothing shows up in dmesg?
The Cyclone5 devkits are pretty cheap if you want to get one.
Are you talking about the DE10-Nano? It seems out of stock everywhere in Europe :/
Thanks! Maxime
The STM32 mux 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.
Since the CLK_SET_RATE_NO_REPARENT flag was already set though, it seems unlikely.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/stm32/clk-stm32-core.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/clk/stm32/clk-stm32-core.c b/drivers/clk/stm32/clk-stm32-core.c index 45a279e73779..3247539683c9 100644 --- a/drivers/clk/stm32/clk-stm32-core.c +++ b/drivers/clk/stm32/clk-stm32-core.c @@ -275,6 +275,7 @@ static int clk_stm32_mux_set_parent(struct clk_hw *hw, u8 index) }
const struct clk_ops clk_stm32_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = clk_stm32_mux_get_parent, .set_parent = clk_stm32_mux_set_parent, };
The Tegra BPMP mux 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 --- drivers/clk/tegra/clk-bpmp.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c index 0ecdffaa6b16..3c12d8c0d28f 100644 --- a/drivers/clk/tegra/clk-bpmp.c +++ b/drivers/clk/tegra/clk-bpmp.c @@ -286,6 +286,7 @@ static const struct clk_ops tegra_bpmp_clk_mux_ops = { .unprepare = tegra_bpmp_clk_unprepare, .is_prepared = tegra_bpmp_clk_is_prepared, .recalc_rate = tegra_bpmp_clk_recalc_rate, + .determine_rate = __clk_mux_determine_rate, .set_parent = tegra_bpmp_clk_set_parent, .get_parent = tegra_bpmp_clk_get_parent, }; @@ -543,10 +544,12 @@ tegra_bpmp_clk_register(struct tegra_bpmp *bpmp, else init.ops = &tegra_bpmp_clk_gate_ops; } else if (info->flags & TEGRA_BPMP_CLK_HAS_MUX) { - if (info->flags & TEGRA_BPMP_CLK_HAS_SET_RATE) + if (info->flags & TEGRA_BPMP_CLK_HAS_SET_RATE) { init.ops = &tegra_bpmp_clk_mux_rate_ops; - else + } else { init.ops = &tegra_bpmp_clk_mux_ops; + init.flags |= CLK_SET_RATE_NO_REPARENT; + } } else { if (info->flags & TEGRA_BPMP_CLK_HAS_SET_RATE) init.ops = &tegra_bpmp_clk_rate_ops;
The Tegra super mux 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 --- drivers/clk/tegra/clk-super.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c index a98a420398fa..8ad62e04fd8b 100644 --- a/drivers/clk/tegra/clk-super.c +++ b/drivers/clk/tegra/clk-super.c @@ -136,6 +136,7 @@ static void clk_super_mux_restore_context(struct clk_hw *hw) }
static const struct clk_ops tegra_clk_super_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = clk_super_get_parent, .set_parent = clk_super_set_parent, .restore_context = clk_super_mux_restore_context, @@ -212,7 +213,7 @@ struct clk *tegra_clk_register_super_mux(const char *name,
init.name = name; init.ops = &tegra_clk_super_mux_ops; - init.flags = flags; + init.flags = flags | CLK_SET_RATE_NO_REPARENT; init.parent_names = parent_names; init.num_parents = num_parents;
The Tegra periph nodiv 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 --- drivers/clk/tegra/clk-periph.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/tegra/clk-periph.c b/drivers/clk/tegra/clk-periph.c index 79ca3aa072b7..367396c62259 100644 --- a/drivers/clk/tegra/clk-periph.c +++ b/drivers/clk/tegra/clk-periph.c @@ -140,6 +140,7 @@ const struct clk_ops tegra_clk_periph_ops = { };
static const struct clk_ops tegra_clk_periph_nodiv_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = clk_periph_get_parent, .set_parent = clk_periph_set_parent, .is_enabled = clk_periph_is_enabled, @@ -170,7 +171,7 @@ static struct clk *_tegra_clk_register_periph(const char *name, bool div = !(periph->gate.flags & TEGRA_PERIPH_NO_DIV);
if (periph->gate.flags & TEGRA_PERIPH_NO_DIV) { - flags |= CLK_SET_RATE_PARENT; + flags |= CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; init.ops = &tegra_clk_periph_nodiv_ops; } else if (periph->gate.flags & TEGRA_PERIPH_NO_GATE) init.ops = &tegra_clk_periph_no_gate_ops;
The UX500 PRCMU "clkout" 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.
Acked-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/ux500/clk-prcmu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/ux500/clk-prcmu.c b/drivers/clk/ux500/clk-prcmu.c index 4deb37f19a7c..7118991f3731 100644 --- a/drivers/clk/ux500/clk-prcmu.c +++ b/drivers/clk/ux500/clk-prcmu.c @@ -344,6 +344,7 @@ static const struct clk_ops clk_prcmu_clkout_ops = { .prepare = clk_prcmu_clkout_prepare, .unprepare = clk_prcmu_clkout_unprepare, .recalc_rate = clk_prcmu_clkout_recalc_rate, + .determine_rate = __clk_mux_determine_rate, .get_parent = clk_prcmu_clkout_get_parent, .set_parent = clk_prcmu_clkout_set_parent, }; @@ -383,7 +384,7 @@ struct clk_hw *clk_reg_prcmu_clkout(const char *name,
clk_prcmu_clkout_init.name = name; clk_prcmu_clkout_init.ops = &clk_prcmu_clkout_ops; - clk_prcmu_clkout_init.flags = CLK_GET_RATE_NOCACHE; + clk_prcmu_clkout_init.flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_NO_REPARENT; clk_prcmu_clkout_init.parent_names = parent_names; clk_prcmu_clkout_init.num_parents = num_parents; clk->hw.init = &clk_prcmu_clkout_init;
On Tue, Apr 4, 2023 at 2:45 PM Maxime Ripard maxime@cerno.tech wrote:
The UX500 PRCMU "clkout" 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.
Not even that.
The parent is selected from the second cell of the device tree specifier, and the divisor from the third cell. See: Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml
So this definitely does not reparent.
Yours, Linus Walleij
The UX500 sysctrl "set_parent" clocks implement 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.
Acked-by: Linus Walleij linus.walleij@linaro.org Reviewed-by: Ulf Hansson ulf.hansson@linaro.org Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/ux500/clk-sysctrl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c index 702f2f8b43fa..d36336665b6d 100644 --- a/drivers/clk/ux500/clk-sysctrl.c +++ b/drivers/clk/ux500/clk-sysctrl.c @@ -110,6 +110,7 @@ static const struct clk_ops clk_sysctrl_gate_fixed_rate_ops = { };
static const struct clk_ops clk_sysctrl_set_parent_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = clk_sysctrl_set_parent, .get_parent = clk_sysctrl_get_parent, }; @@ -220,6 +221,7 @@ struct clk *clk_reg_sysctrl_set_parent(struct device *dev, unsigned long flags) { return clk_reg_sysctrl(dev, name, parent_names, num_parents, - reg_sel, reg_mask, reg_bits, 0, 0, flags, + reg_sel, reg_mask, reg_bits, 0, 0, + flags | CLK_SET_RATE_NO_REPARENT, &clk_sysctrl_set_parent_ops); }
The Tegra sor pad 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 --- drivers/gpu/drm/tegra/sor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index 8af632740673..92084a9a67c5 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -586,6 +586,7 @@ static u8 tegra_clk_sor_pad_get_parent(struct clk_hw *hw) }
static const struct clk_ops tegra_clk_sor_pad_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = tegra_clk_sor_pad_set_parent, .get_parent = tegra_clk_sor_pad_get_parent, }; @@ -604,7 +605,7 @@ static struct clk *tegra_clk_sor_pad_register(struct tegra_sor *sor, pad->sor = sor;
init.name = name; - init.flags = 0; + init.flags = CLK_SET_RATE_NO_REPARENT; init.parent_names = tegra_clk_sor_pad_parents[sor->index]; init.num_parents = ARRAY_SIZE(tegra_clk_sor_pad_parents[sor->index]); init.ops = &tegra_clk_sor_pad_ops;
The Cadence Sierra PLL 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.
Since the CLK_SET_RATE_NO_REPARENT flag was already set though, it seems unlikely.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/phy/cadence/phy-cadence-sierra.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c index ab0a37618ef3..19a50247e305 100644 --- a/drivers/phy/cadence/phy-cadence-sierra.c +++ b/drivers/phy/cadence/phy-cadence-sierra.c @@ -716,6 +716,7 @@ static int cdns_sierra_pll_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops cdns_sierra_pll_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = cdns_sierra_pll_mux_set_parent, .get_parent = cdns_sierra_pll_mux_get_parent, };
The Cadence Torrent refclk 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.
Since the CLK_SET_RATE_NO_REPARENT flag was already set though, it seems unlikely.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/phy/cadence/phy-cadence-torrent.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c index 3831f596d50c..62e59d1bb9c3 100644 --- a/drivers/phy/cadence/phy-cadence-torrent.c +++ b/drivers/phy/cadence/phy-cadence-torrent.c @@ -1861,6 +1861,7 @@ static const struct clk_ops cdns_torrent_refclk_driver_ops = { .enable = cdns_torrent_refclk_driver_enable, .disable = cdns_torrent_refclk_driver_disable, .is_enabled = cdns_torrent_refclk_driver_is_enabled, + .determine_rate = __clk_mux_determine_rate, .set_parent = cdns_torrent_refclk_driver_set_parent, .get_parent = cdns_torrent_refclk_driver_get_parent, };
The TI AM654 SerDes 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.
Since the CLK_SET_RATE_NO_REPARENT flag was already set though, it seems unlikely.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/phy/ti/phy-am654-serdes.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/phy/ti/phy-am654-serdes.c b/drivers/phy/ti/phy-am654-serdes.c index 4ed2d951d3df..3f1d43e8b7ad 100644 --- a/drivers/phy/ti/phy-am654-serdes.c +++ b/drivers/phy/ti/phy-am654-serdes.c @@ -634,6 +634,7 @@ static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops serdes_am654_clk_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = serdes_am654_clk_mux_set_parent, .get_parent = serdes_am654_clk_mux_get_parent, };
The TI J721e Wiz 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.
Since the CLK_SET_RATE_NO_REPARENT flag was already set though, it seems unlikely.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/phy/ti/phy-j721e-wiz.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c index d2fd2143450a..d97f161dfe0c 100644 --- a/drivers/phy/ti/phy-j721e-wiz.c +++ b/drivers/phy/ti/phy-j721e-wiz.c @@ -802,6 +802,7 @@ static int wiz_clk_mux_set_parent(struct clk_hw *hw, u8 index) }
static const struct clk_ops wiz_clk_mux_ops = { + .determine_rate = __clk_mux_determine_rate, .set_parent = wiz_clk_mux_set_parent, .get_parent = wiz_clk_mux_get_parent, };
The Allwinner sun6i RTC 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 --- drivers/rtc/rtc-sun6i.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c index dc76537f1b62..7d975202e0b9 100644 --- a/drivers/rtc/rtc-sun6i.c +++ b/drivers/rtc/rtc-sun6i.c @@ -214,6 +214,7 @@ static int sun6i_rtc_osc_set_parent(struct clk_hw *hw, u8 index)
static const struct clk_ops sun6i_rtc_osc_ops = { .recalc_rate = sun6i_rtc_osc_recalc_rate, + .determine_rate = __clk_mux_determine_rate,
.get_parent = sun6i_rtc_osc_get_parent, .set_parent = sun6i_rtc_osc_set_parent, @@ -227,6 +228,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node, struct clk_init_data init = { .ops = &sun6i_rtc_osc_ops, .name = "losc", + .flags = CLK_SET_RATE_NO_REPARENT, }; const char *iosc_name = "rtc-int-osc"; const char *clkout_name = "osc32k-out";
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(-)
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)
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
The Actions composite divider clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/actions/owl-composite.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/actions/owl-composite.c b/drivers/clk/actions/owl-composite.c index 4c844a5613e4..d66b268563d0 100644 --- a/drivers/clk/actions/owl-composite.c +++ b/drivers/clk/actions/owl-composite.c @@ -53,13 +53,19 @@ static int owl_comp_is_enabled(struct clk_hw *hw) return owl_gate_clk_is_enabled(common, &comp->gate_hw); }
-static long owl_comp_div_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int owl_comp_div_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct owl_composite *comp = hw_to_owl_comp(hw); + long rate;
- return owl_divider_helper_round_rate(&comp->common, &comp->rate.div_hw, - rate, parent_rate); + rate = owl_divider_helper_round_rate(&comp->common, &comp->rate.div_hw, + req->rate, &req->best_parent_rate); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; }
static unsigned long owl_comp_div_recalc_rate(struct clk_hw *hw, @@ -152,7 +158,7 @@ const struct clk_ops owl_comp_div_ops = { .is_enabled = owl_comp_is_enabled,
/* div_ops */ - .round_rate = owl_comp_div_round_rate, + .determine_rate = owl_comp_div_determine_rate, .recalc_rate = owl_comp_div_recalc_rate, .set_rate = owl_comp_div_set_rate, };
The Actions composite factor clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/actions/owl-composite.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/actions/owl-composite.c b/drivers/clk/actions/owl-composite.c index d66b268563d0..3cac8f0a80dc 100644 --- a/drivers/clk/actions/owl-composite.c +++ b/drivers/clk/actions/owl-composite.c @@ -86,14 +86,20 @@ static int owl_comp_div_set_rate(struct clk_hw *hw, unsigned long rate, rate, parent_rate); }
-static long owl_comp_fact_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int owl_comp_fact_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct owl_composite *comp = hw_to_owl_comp(hw); + long rate;
- return owl_factor_helper_round_rate(&comp->common, - &comp->rate.factor_hw, - rate, parent_rate); + rate = owl_factor_helper_round_rate(&comp->common, + &comp->rate.factor_hw, + req->rate, &req->best_parent_rate); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; }
static unsigned long owl_comp_fact_recalc_rate(struct clk_hw *hw, @@ -175,7 +181,7 @@ const struct clk_ops owl_comp_fact_ops = { .is_enabled = owl_comp_is_enabled,
/* fact_ops */ - .round_rate = owl_comp_fact_round_rate, + .determine_rate = owl_comp_fact_determine_rate, .recalc_rate = owl_comp_fact_recalc_rate, .set_rate = owl_comp_fact_set_rate, };
The Atmel SAM9x5 SMD clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/at91/clk-smd.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/clk/at91/clk-smd.c b/drivers/clk/at91/clk-smd.c index 160378438f1b..09c649c8598e 100644 --- a/drivers/clk/at91/clk-smd.c +++ b/drivers/clk/at91/clk-smd.c @@ -36,26 +36,31 @@ static unsigned long at91sam9x5_clk_smd_recalc_rate(struct clk_hw *hw, return parent_rate / (smddiv + 1); }
-static long at91sam9x5_clk_smd_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int at91sam9x5_clk_smd_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { unsigned long div; unsigned long bestrate; unsigned long tmp;
- if (rate >= *parent_rate) - return *parent_rate; + if (req->rate >= req->best_parent_rate) { + req->rate = req->best_parent_rate; + return 0; + }
- div = *parent_rate / rate; - if (div > SMD_MAX_DIV) - return *parent_rate / (SMD_MAX_DIV + 1); + div = req->best_parent_rate / req->rate; + if (div > SMD_MAX_DIV) { + req->rate = req->best_parent_rate / (SMD_MAX_DIV + 1); + return 0; + }
- bestrate = *parent_rate / div; - tmp = *parent_rate / (div + 1); - if (bestrate - rate > rate - tmp) + bestrate = req->best_parent_rate / div; + tmp = req->best_parent_rate / (div + 1); + if (bestrate - req->rate > req->rate - tmp) bestrate = tmp;
- return bestrate; + req->rate = bestrate; + return 0; }
static int at91sam9x5_clk_smd_set_parent(struct clk_hw *hw, u8 index) @@ -98,7 +103,7 @@ static int at91sam9x5_clk_smd_set_rate(struct clk_hw *hw, unsigned long rate,
static const struct clk_ops at91sam9x5_smd_ops = { .recalc_rate = at91sam9x5_clk_smd_recalc_rate, - .round_rate = at91sam9x5_clk_smd_round_rate, + .determine_rate = at91sam9x5_clk_smd_determine_rate, .get_parent = at91sam9x5_clk_smd_get_parent, .set_parent = at91sam9x5_clk_smd_set_parent, .set_rate = at91sam9x5_clk_smd_set_rate,
On 04.04.2023 13:11, Maxime Ripard wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
The Atmel SAM9x5 SMD clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Claudiu Beznea claudiu.beznea@microchip.com
Tested-by: Claudiu Beznea claudiu.beznea@microchip.com
drivers/clk/at91/clk-smd.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/clk/at91/clk-smd.c b/drivers/clk/at91/clk-smd.c index 160378438f1b..09c649c8598e 100644 --- a/drivers/clk/at91/clk-smd.c +++ b/drivers/clk/at91/clk-smd.c @@ -36,26 +36,31 @@ static unsigned long at91sam9x5_clk_smd_recalc_rate(struct clk_hw *hw, return parent_rate / (smddiv + 1); }
-static long at91sam9x5_clk_smd_round_rate(struct clk_hw *hw, unsigned long rate,
unsigned long *parent_rate)+static int at91sam9x5_clk_smd_determine_rate(struct clk_hw *hw,
struct clk_rate_request *req){ unsigned long div; unsigned long bestrate; unsigned long tmp;
if (rate >= *parent_rate)return *parent_rate;
if (req->rate >= req->best_parent_rate) {req->rate = req->best_parent_rate;return 0;}
div = *parent_rate / rate;if (div > SMD_MAX_DIV)return *parent_rate / (SMD_MAX_DIV + 1);
div = req->best_parent_rate / req->rate;if (div > SMD_MAX_DIV) {req->rate = req->best_parent_rate / (SMD_MAX_DIV + 1);return 0;}
bestrate = *parent_rate / div;tmp = *parent_rate / (div + 1);if (bestrate - rate > rate - tmp)
bestrate = req->best_parent_rate / div;tmp = req->best_parent_rate / (div + 1);if (bestrate - req->rate > req->rate - tmp) bestrate = tmp;
return bestrate;
req->rate = bestrate;return 0;}
static int at91sam9x5_clk_smd_set_parent(struct clk_hw *hw, u8 index) @@ -98,7 +103,7 @@ static int at91sam9x5_clk_smd_set_rate(struct clk_hw *hw, unsigned long rate,
static const struct clk_ops at91sam9x5_smd_ops = { .recalc_rate = at91sam9x5_clk_smd_recalc_rate,
.round_rate = at91sam9x5_clk_smd_round_rate,
.determine_rate = at91sam9x5_clk_smd_determine_rate, .get_parent = at91sam9x5_clk_smd_get_parent, .set_parent = at91sam9x5_clk_smd_set_parent, .set_rate = at91sam9x5_clk_smd_set_rate,-- 2.39.2
The AXI clkgen clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-axi-clkgen.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk-axi-clkgen.c b/drivers/clk/clk-axi-clkgen.c index 671bee55ceb3..dded52f9c774 100644 --- a/drivers/clk/clk-axi-clkgen.c +++ b/drivers/clk/clk-axi-clkgen.c @@ -384,23 +384,25 @@ static int axi_clkgen_set_rate(struct clk_hw *clk_hw, return 0; }
-static long axi_clkgen_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int axi_clkgen_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct axi_clkgen *axi_clkgen = clk_hw_to_axi_clkgen(hw); const struct axi_clkgen_limits *limits = &axi_clkgen->limits; unsigned int d, m, dout; unsigned long long tmp;
- axi_clkgen_calc_params(limits, *parent_rate, rate, &d, &m, &dout); + axi_clkgen_calc_params(limits, req->best_parent_rate, req->rate, + &d, &m, &dout);
if (d == 0 || dout == 0 || m == 0) return -EINVAL;
- tmp = (unsigned long long)*parent_rate * m; + tmp = (unsigned long long)req->best_parent_rate * m; tmp = DIV_ROUND_CLOSEST_ULL(tmp, dout * d);
- return min_t(unsigned long long, tmp, LONG_MAX); + req->rate = min_t(unsigned long long, tmp, LONG_MAX); + return 0; }
static unsigned int axi_clkgen_get_div(struct axi_clkgen *axi_clkgen, @@ -495,7 +497,7 @@ static u8 axi_clkgen_get_parent(struct clk_hw *clk_hw)
static const struct clk_ops axi_clkgen_ops = { .recalc_rate = axi_clkgen_recalc_rate, - .round_rate = axi_clkgen_round_rate, + .determine_rate = axi_clkgen_determine_rate, .set_rate = axi_clkgen_set_rate, .enable = axi_clkgen_enable, .disable = axi_clkgen_disable,
The cdce706 divider clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-cdce706.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/clk-cdce706.c b/drivers/clk/clk-cdce706.c index dc046bbf83a1..a53769d239a9 100644 --- a/drivers/clk/clk-cdce706.c +++ b/drivers/clk/clk-cdce706.c @@ -288,18 +288,19 @@ static unsigned long cdce706_divider_recalc_rate(struct clk_hw *hw, return 0; }
-static long cdce706_divider_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int cdce706_divider_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct cdce706_hw_data *hwd = to_hw_data(hw); struct cdce706_dev_data *cdce = hwd->dev_data; + unsigned long rate = req->rate; unsigned long mul, div;
dev_dbg(&hwd->dev_data->client->dev, "%s, rate: %lu, parent_rate: %lu\n", - __func__, rate, *parent_rate); + __func__, rate, req->best_parent_rate);
- rational_best_approximation(rate, *parent_rate, + rational_best_approximation(rate, req->best_parent_rate, 1, CDCE706_DIVIDER_DIVIDER_MAX, &mul, &div); if (!mul) @@ -344,8 +345,8 @@ static long cdce706_divider_round_rate(struct clk_hw *hw, unsigned long rate,
dev_dbg(&hwd->dev_data->client->dev, "%s, altering parent rate: %lu -> %lu\n", - __func__, *parent_rate, rate * div); - *parent_rate = rate * div; + __func__, req->best_parent_rate, rate * div); + req->best_parent_rate = rate * div; } hwd->div = div;
@@ -353,7 +354,8 @@ static long cdce706_divider_round_rate(struct clk_hw *hw, unsigned long rate, "%s, divider: %d, div: %lu\n", __func__, hwd->idx, div);
- return *parent_rate / div; + req->rate = req->best_parent_rate / div; + return 0; }
static int cdce706_divider_set_rate(struct clk_hw *hw, unsigned long rate, @@ -375,7 +377,7 @@ static const struct clk_ops cdce706_divider_ops = { .set_parent = cdce706_divider_set_parent, .get_parent = cdce706_divider_get_parent, .recalc_rate = cdce706_divider_recalc_rate, - .round_rate = cdce706_divider_round_rate, + .determine_rate = cdce706_divider_determine_rate, .set_rate = cdce706_divider_set_rate, };
The cdce706 clkout clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-cdce706.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/clk-cdce706.c b/drivers/clk/clk-cdce706.c index a53769d239a9..0a560b4d295e 100644 --- a/drivers/clk/clk-cdce706.c +++ b/drivers/clk/clk-cdce706.c @@ -423,11 +423,12 @@ static unsigned long cdce706_clkout_recalc_rate(struct clk_hw *hw, return parent_rate; }
-static long cdce706_clkout_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int cdce706_clkout_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { - *parent_rate = rate; - return rate; + req->best_parent_rate = req->rate; + + return 0; }
static int cdce706_clkout_set_rate(struct clk_hw *hw, unsigned long rate, @@ -442,7 +443,7 @@ static const struct clk_ops cdce706_clkout_ops = { .set_parent = cdce706_clkout_set_parent, .get_parent = cdce706_clkout_get_parent, .recalc_rate = cdce706_clkout_recalc_rate, - .round_rate = cdce706_clkout_round_rate, + .determine_rate = cdce706_clkout_determine_rate, .set_rate = cdce706_clkout_set_rate, };
The SI5341 output clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-si5341.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c index 259861aa2e2f..14792d5ffb4f 100644 --- a/drivers/clk/clk-si5341.c +++ b/drivers/clk/clk-si5341.c @@ -828,19 +828,20 @@ static unsigned long si5341_output_clk_recalc_rate(struct clk_hw *hw, return parent_rate / r_divider; }
-static long si5341_output_clk_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int si5341_output_clk_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { + unsigned long rate = req->rate; unsigned long r;
if (!rate) return 0;
- r = *parent_rate >> 1; + r = req->best_parent_rate >> 1;
/* If rate is an even divisor, no changes to parent required */ if (r && !(r % rate)) - return (long)rate; + return 0;
if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) { if (rate > 200000000) { @@ -850,14 +851,15 @@ static long si5341_output_clk_round_rate(struct clk_hw *hw, unsigned long rate, /* Take a parent frequency near 400 MHz */ r = (400000000u / rate) & ~1; } - *parent_rate = r * rate; + req->best_parent_rate = r * rate; } else { /* We cannot change our parent's rate, report what we can do */ r /= rate; - rate = *parent_rate / (r << 1); + rate = req->best_parent_rate / (r << 1); }
- return rate; + req->rate = rate; + return 0; }
static int si5341_output_clk_set_rate(struct clk_hw *hw, unsigned long rate, @@ -930,7 +932,7 @@ static const struct clk_ops si5341_output_clk_ops = { .prepare = si5341_output_clk_prepare, .unprepare = si5341_output_clk_unprepare, .recalc_rate = si5341_output_clk_recalc_rate, - .round_rate = si5341_output_clk_round_rate, + .determine_rate = si5341_output_clk_determine_rate, .set_rate = si5341_output_clk_set_rate, .set_parent = si5341_output_set_parent, .get_parent = si5341_output_get_parent,
The SI5351 PLL clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-si5351.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c index 9e939c98a455..fcf5785ba4ce 100644 --- a/drivers/clk/clk-si5351.c +++ b/drivers/clk/clk-si5351.c @@ -442,11 +442,12 @@ static unsigned long si5351_pll_recalc_rate(struct clk_hw *hw, return (unsigned long)rate; }
-static long si5351_pll_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int si5351_pll_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct si5351_hw_data *hwdata = container_of(hw, struct si5351_hw_data, hw); + unsigned long rate = req->rate; unsigned long rfrac, denom, a, b, c; unsigned long long lltmp;
@@ -456,18 +457,18 @@ static long si5351_pll_round_rate(struct clk_hw *hw, unsigned long rate, rate = SI5351_PLL_VCO_MAX;
/* determine integer part of feedback equation */ - a = rate / *parent_rate; + a = rate / req->best_parent_rate;
if (a < SI5351_PLL_A_MIN) - rate = *parent_rate * SI5351_PLL_A_MIN; + rate = req->best_parent_rate * SI5351_PLL_A_MIN; if (a > SI5351_PLL_A_MAX) - rate = *parent_rate * SI5351_PLL_A_MAX; + rate = req->best_parent_rate * SI5351_PLL_A_MAX;
/* find best approximation for b/c = fVCO mod fIN */ denom = 1000 * 1000; - lltmp = rate % (*parent_rate); + lltmp = rate % (req->best_parent_rate); lltmp *= denom; - do_div(lltmp, *parent_rate); + do_div(lltmp, req->best_parent_rate); rfrac = (unsigned long)lltmp;
b = 0; @@ -484,19 +485,20 @@ static long si5351_pll_round_rate(struct clk_hw *hw, unsigned long rate, hwdata->params.p1 -= 512;
/* recalculate rate by fIN * (a + b/c) */ - lltmp = *parent_rate; + lltmp = req->best_parent_rate; lltmp *= b; do_div(lltmp, c);
rate = (unsigned long)lltmp; - rate += *parent_rate * a; + rate += req->best_parent_rate * a;
dev_dbg(&hwdata->drvdata->client->dev, "%s - %s: a = %lu, b = %lu, c = %lu, parent_rate = %lu, rate = %lu\n", __func__, clk_hw_get_name(hw), a, b, c, - *parent_rate, rate); + req->best_parent_rate, rate);
- return rate; + req->rate = rate; + return 0; }
static int si5351_pll_set_rate(struct clk_hw *hw, unsigned long rate, @@ -533,7 +535,7 @@ static const struct clk_ops si5351_pll_ops = { .set_parent = si5351_pll_set_parent, .get_parent = si5351_pll_get_parent, .recalc_rate = si5351_pll_recalc_rate, - .round_rate = si5351_pll_round_rate, + .determine_rate = si5351_pll_determine_rate, .set_rate = si5351_pll_set_rate, };
The SI5351 msynth clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-si5351.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c index fcf5785ba4ce..bfab05f4fe28 100644 --- a/drivers/clk/clk-si5351.c +++ b/drivers/clk/clk-si5351.c @@ -642,11 +642,12 @@ static unsigned long si5351_msynth_recalc_rate(struct clk_hw *hw, return (unsigned long)rate; }
-static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int si5351_msynth_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct si5351_hw_data *hwdata = container_of(hw, struct si5351_hw_data, hw); + unsigned long rate = req->rate; unsigned long long lltmp; unsigned long a, b, c; int divby4; @@ -681,10 +682,10 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate, b = 0; c = 1;
- *parent_rate = a * rate; + req->best_parent_rate = a * rate; } else if (hwdata->num >= 6) { /* determine the closest integer divider */ - a = DIV_ROUND_CLOSEST(*parent_rate, rate); + a = DIV_ROUND_CLOSEST(req->best_parent_rate, rate); if (a < SI5351_MULTISYNTH_A_MIN) a = SI5351_MULTISYNTH_A_MIN; if (a > SI5351_MULTISYNTH67_A_MAX) @@ -702,7 +703,7 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate, }
/* determine integer part of divider equation */ - a = *parent_rate / rate; + a = req->best_parent_rate / rate; if (a < SI5351_MULTISYNTH_A_MIN) a = SI5351_MULTISYNTH_A_MIN; if (a > SI5351_MULTISYNTH_A_MAX) @@ -710,7 +711,7 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
/* find best approximation for b/c = fVCO mod fOUT */ denom = 1000 * 1000; - lltmp = (*parent_rate) % rate; + lltmp = req->best_parent_rate % rate; lltmp *= denom; do_div(lltmp, rate); rfrac = (unsigned long)lltmp; @@ -724,7 +725,7 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate, }
/* recalculate rate by fOUT = fIN / (a + b/c) */ - lltmp = *parent_rate; + lltmp = req->best_parent_rate; lltmp *= c; do_div(lltmp, a * c + b); rate = (unsigned long)lltmp; @@ -749,9 +750,11 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate, dev_dbg(&hwdata->drvdata->client->dev, "%s - %s: a = %lu, b = %lu, c = %lu, divby4 = %d, parent_rate = %lu, rate = %lu\n", __func__, clk_hw_get_name(hw), a, b, c, divby4, - *parent_rate, rate); + req->best_parent_rate, rate);
- return rate; + req->rate = rate; + + return 0; }
static int si5351_msynth_set_rate(struct clk_hw *hw, unsigned long rate, @@ -791,7 +794,7 @@ static const struct clk_ops si5351_msynth_ops = { .set_parent = si5351_msynth_set_parent, .get_parent = si5351_msynth_get_parent, .recalc_rate = si5351_msynth_recalc_rate, - .round_rate = si5351_msynth_round_rate, + .determine_rate = si5351_msynth_determine_rate, .set_rate = si5351_msynth_set_rate, };
The SI5351 clkout clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk-si5351.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c index bfab05f4fe28..11aaa934da29 100644 --- a/drivers/clk/clk-si5351.c +++ b/drivers/clk/clk-si5351.c @@ -1037,11 +1037,12 @@ static unsigned long si5351_clkout_recalc_rate(struct clk_hw *hw, return parent_rate >> rdiv; }
-static long si5351_clkout_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int si5351_clkout_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct si5351_hw_data *hwdata = container_of(hw, struct si5351_hw_data, hw); + unsigned long rate = req->rate; unsigned char rdiv;
/* clkout6/7 can only handle output freqencies < 150MHz */ @@ -1063,13 +1064,13 @@ static long si5351_clkout_round_rate(struct clk_hw *hw, unsigned long rate, rdiv += 1; rate *= 2; } - *parent_rate = rate; + req->best_parent_rate = rate; } else { unsigned long new_rate, new_err, err;
/* round to closed rdiv */ rdiv = SI5351_OUTPUT_CLK_DIV_1; - new_rate = *parent_rate; + new_rate = req->best_parent_rate; err = abs(new_rate - rate); do { new_rate >>= 1; @@ -1080,14 +1081,15 @@ static long si5351_clkout_round_rate(struct clk_hw *hw, unsigned long rate, err = new_err; } while (1); } - rate = *parent_rate >> rdiv; + rate = req->best_parent_rate >> rdiv;
dev_dbg(&hwdata->drvdata->client->dev, "%s - %s: rdiv = %u, parent_rate = %lu, rate = %lu\n", __func__, clk_hw_get_name(hw), (1 << rdiv), - *parent_rate, rate); + req->best_parent_rate, rate);
- return rate; + req->rate = rate; + return 0; }
static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate, @@ -1147,7 +1149,7 @@ static const struct clk_ops si5351_clkout_ops = { .set_parent = si5351_clkout_set_parent, .get_parent = si5351_clkout_get_parent, .recalc_rate = si5351_clkout_recalc_rate, - .round_rate = si5351_clkout_round_rate, + .determine_rate = si5351_clkout_determine_rate, .set_rate = si5351_clkout_set_rate, };
The TI DA8xx USB0 clk48 clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/davinci/da8xx-cfgchip.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c index 4c1cc59bba53..f60c97091818 100644 --- a/drivers/clk/davinci/da8xx-cfgchip.c +++ b/drivers/clk/davinci/da8xx-cfgchip.c @@ -462,10 +462,12 @@ static unsigned long da8xx_usb0_clk48_recalc_rate(struct clk_hw *hw, return 48000000; }
-static long da8xx_usb0_clk48_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int da8xx_usb0_clk48_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { - return 48000000; + req->rate = 48000000; + + return 0; }
static int da8xx_usb0_clk48_set_parent(struct clk_hw *hw, u8 index) @@ -494,7 +496,7 @@ static const struct clk_ops da8xx_usb0_clk48_ops = { .disable = da8xx_usb0_clk48_disable, .is_enabled = da8xx_usb0_clk48_is_enabled, .recalc_rate = da8xx_usb0_clk48_recalc_rate, - .round_rate = da8xx_usb0_clk48_round_rate, + .determine_rate = da8xx_usb0_clk48_determine_rate, .set_parent = da8xx_usb0_clk48_set_parent, .get_parent = da8xx_usb0_clk48_get_parent, };
On 4/4/23 5:11 AM, Maxime Ripard wrote:
The TI DA8xx USB0 clk48 clocks 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.
As mentioned in my previous review, parent is selected by device tree and should never be changed after init.
Hi David,
On Wed, Apr 05, 2023 at 10:03:24AM -0500, David Lechner wrote:
On 4/4/23 5:11 AM, Maxime Ripard wrote:
The TI DA8xx USB0 clk48 clocks 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.
As mentioned in my previous review, parent is selected by device tree and should never be changed after init.
Great minds think alike then, because the driver implements exactly that, either before or after that patch.
That patch makes the current behaviour explicit but doesn't change it in any way.
So I guess that means that I can add your Acked-by on the three patches you reviewed with the same message?
Maxime
On 4/5/23 10:22 AM, Maxime Ripard wrote:
Hi David,
On Wed, Apr 05, 2023 at 10:03:24AM -0500, David Lechner wrote:
On 4/4/23 5:11 AM, Maxime Ripard wrote:
The TI DA8xx USB0 clk48 clocks 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.
As mentioned in my previous review, parent is selected by device tree and should never be changed after init.
Great minds think alike then, because the driver implements exactly that, either before or after that patch.
That patch makes the current behaviour explicit but doesn't change it in any way.
So I guess that means that I can add your Acked-by on the three patches you reviewed with the same message?
Maxime
Yes, preferably with a simplified commit message.
Acked-by: David Lechner david@lechnology.com
The iMX SCU clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. The round_rate() implementation being shared with other clocks, it's not removed.
And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/imx/clk-scu.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c index 66e49fea5f8a..bbdc1b23f6f5 100644 --- a/drivers/clk/imx/clk-scu.c +++ b/drivers/clk/imx/clk-scu.c @@ -250,6 +250,23 @@ static unsigned long clk_scu_recalc_rate(struct clk_hw *hw, return le32_to_cpu(msg.data.resp.rate); }
+/* + * clk_scu_determine_rate - Returns the closest rate for a SCU clock + * @hw: clock to round rate for + * @req: clock rate request + * + * Returns 0 on success, a negative error on failure + */ +static int clk_scu_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + /* + * Assume we support all the requested rate and let the SCU firmware + * to handle the left work + */ + return 0; +} + /* * clk_scu_round_rate - Round clock rate for a SCU clock * @hw: clock to round rate for @@ -425,7 +442,7 @@ static void clk_scu_unprepare(struct clk_hw *hw)
static const struct clk_ops clk_scu_ops = { .recalc_rate = clk_scu_recalc_rate, - .round_rate = clk_scu_round_rate, + .determine_rate = clk_scu_determine_rate, .set_rate = clk_scu_set_rate, .get_parent = clk_scu_get_parent, .set_parent = clk_scu_set_parent,
The Ingenic CGU clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/ingenic/cgu.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c index 1f7ba30f5a1b..0c9c8344ad11 100644 --- a/drivers/clk/ingenic/cgu.c +++ b/drivers/clk/ingenic/cgu.c @@ -491,22 +491,23 @@ ingenic_clk_calc_div(struct clk_hw *hw, return div; }
-static long -ingenic_clk_round_rate(struct clk_hw *hw, unsigned long req_rate, - unsigned long *parent_rate) +static int ingenic_clk_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct ingenic_clk *ingenic_clk = to_ingenic_clk(hw); const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk); unsigned int div = 1;
if (clk_info->type & CGU_CLK_DIV) - div = ingenic_clk_calc_div(hw, clk_info, *parent_rate, req_rate); + div = ingenic_clk_calc_div(hw, clk_info, req->best_parent_rate, + req->rate); else if (clk_info->type & CGU_CLK_FIXDIV) div = clk_info->fixdiv.div; else if (clk_hw_can_set_rate_parent(hw)) - *parent_rate = req_rate; + req->best_parent_rate = req->rate;
- return DIV_ROUND_UP(*parent_rate, div); + req->rate = DIV_ROUND_UP(req->best_parent_rate, div); + return 0; }
static inline int ingenic_clk_check_stable(struct ingenic_cgu *cgu, @@ -626,7 +627,7 @@ static const struct clk_ops ingenic_clk_ops = { .set_parent = ingenic_clk_set_parent,
.recalc_rate = ingenic_clk_recalc_rate, - .round_rate = ingenic_clk_round_rate, + .determine_rate = ingenic_clk_determine_rate, .set_rate = ingenic_clk_set_rate,
.enable = ingenic_clk_enable,
Hi Maxime,
Le mardi 04 avril 2023 à 12:11 +0200, Maxime Ripard a écrit :
The Ingenic CGU clocks 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().
As I said in the v2 (IIRC), clk_set_parent() is used when re-parenting from the device tree.
The driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
So just to be sure, this patch won't make clk_set_rate() automatically switch parents, right?
Allowing automatic re-parenting sounds like a huge can of worms...
Cheers, -Paul
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/clk/ingenic/cgu.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c index 1f7ba30f5a1b..0c9c8344ad11 100644 --- a/drivers/clk/ingenic/cgu.c +++ b/drivers/clk/ingenic/cgu.c @@ -491,22 +491,23 @@ ingenic_clk_calc_div(struct clk_hw *hw, return div; } -static long -ingenic_clk_round_rate(struct clk_hw *hw, unsigned long req_rate, - unsigned long *parent_rate) +static int ingenic_clk_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct ingenic_clk *ingenic_clk = to_ingenic_clk(hw); const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk); unsigned int div = 1; if (clk_info->type & CGU_CLK_DIV) - div = ingenic_clk_calc_div(hw, clk_info, *parent_rate, req_rate); + div = ingenic_clk_calc_div(hw, clk_info, req-
best_parent_rate,
+ req->rate); else if (clk_info->type & CGU_CLK_FIXDIV) div = clk_info->fixdiv.div; else if (clk_hw_can_set_rate_parent(hw)) - *parent_rate = req_rate; + req->best_parent_rate = req->rate; - return DIV_ROUND_UP(*parent_rate, div); + req->rate = DIV_ROUND_UP(req->best_parent_rate, div); + return 0; } static inline int ingenic_clk_check_stable(struct ingenic_cgu *cgu, @@ -626,7 +627,7 @@ static const struct clk_ops ingenic_clk_ops = { .set_parent = ingenic_clk_set_parent, .recalc_rate = ingenic_clk_recalc_rate, - .round_rate = ingenic_clk_round_rate, + .determine_rate = ingenic_clk_determine_rate, .set_rate = ingenic_clk_set_rate, .enable = ingenic_clk_enable,
Hi Paul,
On Wed, Apr 05, 2023 at 03:04:05PM +0200, Paul Cercueil wrote:
Le mardi 04 avril 2023 à 12:11 +0200, Maxime Ripard a écrit :
The Ingenic CGU clocks 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().
As I said in the v2 (IIRC), clk_set_parent() is used when re-parenting from the device tree.
Yep, it's indeed an oversight in the commit log.
The driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
So just to be sure, this patch won't make clk_set_rate() automatically switch parents, right?
Allowing automatic re-parenting sounds like a huge can of worms...
The behaviour is strictly equivalent before that patch and after: the driver will not reparent on a rate change. It just makes it explicit.
Maxime
The Ingenic TCU clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/ingenic/tcu.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c index d5544cbc5c48..7d04ef40b7cf 100644 --- a/drivers/clk/ingenic/tcu.c +++ b/drivers/clk/ingenic/tcu.c @@ -178,18 +178,21 @@ static u8 ingenic_tcu_get_prescale(unsigned long rate, unsigned long req_rate) return 5; /* /1024 divider */ }
-static long ingenic_tcu_round_rate(struct clk_hw *hw, unsigned long req_rate, - unsigned long *parent_rate) +static int ingenic_tcu_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { - unsigned long rate = *parent_rate; + unsigned long rate = req->best_parent_rate; u8 prescale;
- if (req_rate > rate) - return rate; + if (req->rate > rate) { + req->rate = rate; + return 0; + }
- prescale = ingenic_tcu_get_prescale(rate, req_rate); + prescale = ingenic_tcu_get_prescale(rate, req->rate);
- return rate >> (prescale * 2); + req->rate = rate >> (prescale * 2); + return 0; }
static int ingenic_tcu_set_rate(struct clk_hw *hw, unsigned long req_rate, @@ -219,7 +222,7 @@ static const struct clk_ops ingenic_tcu_clk_ops = { .set_parent = ingenic_tcu_set_parent,
.recalc_rate = ingenic_tcu_recalc_rate, - .round_rate = ingenic_tcu_round_rate, + .determine_rate = ingenic_tcu_determine_rate, .set_rate = ingenic_tcu_set_rate,
.enable = ingenic_tcu_enable,
The Spreadtrum composite clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Acked-by: Chunyan Zhang zhang.lyra@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/sprd/composite.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/sprd/composite.c b/drivers/clk/sprd/composite.c index ebb644820b1e..d3a852720c07 100644 --- a/drivers/clk/sprd/composite.c +++ b/drivers/clk/sprd/composite.c @@ -9,13 +9,19 @@
#include "composite.h"
-static long sprd_comp_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int sprd_comp_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct sprd_comp *cc = hw_to_sprd_comp(hw); + unsigned long rate;
- return sprd_div_helper_round_rate(&cc->common, &cc->div, - rate, parent_rate); + rate = sprd_div_helper_round_rate(&cc->common, &cc->div, + req->rate, &req->best_parent_rate); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; }
static unsigned long sprd_comp_recalc_rate(struct clk_hw *hw, @@ -53,7 +59,7 @@ const struct clk_ops sprd_comp_ops = { .get_parent = sprd_comp_get_parent, .set_parent = sprd_comp_set_parent,
- .round_rate = sprd_comp_round_rate, + .determine_rate = sprd_comp_determine_rate, .recalc_rate = sprd_comp_recalc_rate, .set_rate = sprd_comp_set_rate, };
The ST Flexgen clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/st/clk-flexgen.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/st/clk-flexgen.c b/drivers/clk/st/clk-flexgen.c index 7ae4f656191e..5292208c4dd8 100644 --- a/drivers/clk/st/clk-flexgen.c +++ b/drivers/clk/st/clk-flexgen.c @@ -119,20 +119,21 @@ clk_best_div(unsigned long parent_rate, unsigned long rate) return parent_rate / rate + ((rate > (2*(parent_rate % rate))) ? 0 : 1); }
-static long flexgen_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *prate) +static int flexgen_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { unsigned long div;
/* Round div according to exact prate and wished rate */ - div = clk_best_div(*prate, rate); + div = clk_best_div(req->best_parent_rate, req->rate);
if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) { - *prate = rate * div; - return rate; + req->best_parent_rate = req->rate * div; + return 0; }
- return *prate / div; + req->rate = req->best_parent_rate / div; + return 0; }
static unsigned long flexgen_recalc_rate(struct clk_hw *hw, @@ -197,7 +198,7 @@ static const struct clk_ops flexgen_ops = { .is_enabled = flexgen_is_enabled, .get_parent = flexgen_get_parent, .set_parent = flexgen_set_parent, - .round_rate = flexgen_round_rate, + .determine_rate = flexgen_determine_rate, .recalc_rate = flexgen_recalc_rate, .set_rate = flexgen_set_rate, };
The STM32 composite clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/stm32/clk-stm32-core.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/clk/stm32/clk-stm32-core.c b/drivers/clk/stm32/clk-stm32-core.c index 3247539683c9..d5aa09e9fce4 100644 --- a/drivers/clk/stm32/clk-stm32-core.c +++ b/drivers/clk/stm32/clk-stm32-core.c @@ -426,15 +426,15 @@ static unsigned long clk_stm32_composite_recalc_rate(struct clk_hw *hw, composite->div_id, parent_rate); }
-static long clk_stm32_composite_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *prate) +static int clk_stm32_composite_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct clk_stm32_composite *composite = to_clk_stm32_composite(hw); - const struct stm32_div_cfg *divider; + unsigned long rate;
if (composite->div_id == NO_STM32_DIV) - return rate; + return 0;
divider = &composite->clock_data->dividers[composite->div_id];
@@ -445,14 +445,24 @@ static long clk_stm32_composite_round_rate(struct clk_hw *hw, unsigned long rate val = readl(composite->base + divider->offset) >> divider->shift; val &= clk_div_mask(divider->width);
- return divider_ro_round_rate(hw, rate, prate, divider->table, - divider->width, divider->flags, - val); + rate = divider_ro_round_rate(hw, req->rate, &req->best_parent_rate, + divider->table, divider->width, divider->flags, + val); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; }
- return divider_round_rate_parent(hw, clk_hw_get_parent(hw), - rate, prate, divider->table, - divider->width, divider->flags); + rate = divider_round_rate_parent(hw, clk_hw_get_parent(hw), + req->rate, &req->best_parent_rate, + divider->table, divider->width, divider->flags); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; }
static u8 clk_stm32_composite_get_parent(struct clk_hw *hw) @@ -602,7 +612,7 @@ static void clk_stm32_composite_disable_unused(struct clk_hw *hw) const struct clk_ops clk_stm32_composite_ops = { .set_rate = clk_stm32_composite_set_rate, .recalc_rate = clk_stm32_composite_recalc_rate, - .round_rate = clk_stm32_composite_round_rate, + .determine_rate = clk_stm32_composite_determine_rate, .get_parent = clk_stm32_composite_get_parent, .set_parent = clk_stm32_composite_set_parent, .enable = clk_stm32_composite_gate_enable,
The Tegra periph clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/tegra/clk-periph.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/tegra/clk-periph.c b/drivers/clk/tegra/clk-periph.c index 367396c62259..ce8cab5f1978 100644 --- a/drivers/clk/tegra/clk-periph.c +++ b/drivers/clk/tegra/clk-periph.c @@ -45,16 +45,22 @@ static unsigned long clk_periph_recalc_rate(struct clk_hw *hw, return div_ops->recalc_rate(div_hw, parent_rate); }
-static long clk_periph_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *prate) +static int clk_periph_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct tegra_clk_periph *periph = to_clk_periph(hw); const struct clk_ops *div_ops = periph->div_ops; struct clk_hw *div_hw = &periph->divider.hw; + unsigned long rate;
__clk_hw_set_clk(div_hw, hw);
- return div_ops->round_rate(div_hw, rate, prate); + rate = div_ops->round_rate(div_hw, req->rate, &req->best_parent_rate); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; }
static int clk_periph_set_rate(struct clk_hw *hw, unsigned long rate, @@ -130,7 +136,7 @@ const struct clk_ops tegra_clk_periph_ops = { .get_parent = clk_periph_get_parent, .set_parent = clk_periph_set_parent, .recalc_rate = clk_periph_recalc_rate, - .round_rate = clk_periph_round_rate, + .determine_rate = clk_periph_determine_rate, .set_rate = clk_periph_set_rate, .is_enabled = clk_periph_is_enabled, .enable = clk_periph_enable, @@ -154,7 +160,7 @@ static const struct clk_ops tegra_clk_periph_no_gate_ops = { .get_parent = clk_periph_get_parent, .set_parent = clk_periph_set_parent, .recalc_rate = clk_periph_recalc_rate, - .round_rate = clk_periph_round_rate, + .determine_rate = clk_periph_determine_rate, .set_rate = clk_periph_set_rate, .restore_context = clk_periph_restore_context, };
The Tegra super clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/tegra/clk-super.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c index 8ad62e04fd8b..c035f0eb030d 100644 --- a/drivers/clk/tegra/clk-super.c +++ b/drivers/clk/tegra/clk-super.c @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { .restore_context = clk_super_mux_restore_context, };
-static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int clk_super_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct tegra_clk_super_mux *super = to_clk_super_mux(hw); struct clk_hw *div_hw = &super->frac_div.hw; + unsigned long rate;
__clk_hw_set_clk(div_hw, hw);
- return super->div_ops->round_rate(div_hw, rate, parent_rate); + rate = super->div_ops->round_rate(div_hw, req->rate, + &req->best_parent_rate); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; }
static unsigned long clk_super_recalc_rate(struct clk_hw *hw, @@ -193,7 +200,7 @@ const struct clk_ops tegra_clk_super_ops = { .get_parent = clk_super_get_parent, .set_parent = clk_super_set_parent, .set_rate = clk_super_set_rate, - .round_rate = clk_super_round_rate, + .determine_rate = clk_super_determine_rate, .recalc_rate = clk_super_recalc_rate, .restore_context = clk_super_restore_context, };
The tlv320aic32x4 PLL clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- sound/soc/codecs/tlv320aic32x4-clk.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/tlv320aic32x4-clk.c b/sound/soc/codecs/tlv320aic32x4-clk.c index 65b72373cb95..d8b8ea3eaa12 100644 --- a/sound/soc/codecs/tlv320aic32x4-clk.c +++ b/sound/soc/codecs/tlv320aic32x4-clk.c @@ -205,18 +205,23 @@ static unsigned long clk_aic32x4_pll_recalc_rate(struct clk_hw *hw, return clk_aic32x4_pll_calc_rate(&settings, parent_rate); }
-static long clk_aic32x4_pll_round_rate(struct clk_hw *hw, - unsigned long rate, - unsigned long *parent_rate) +static int clk_aic32x4_pll_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct clk_aic32x4_pll_muldiv settings; + unsigned long rate; int ret;
- ret = clk_aic32x4_pll_calc_muldiv(&settings, rate, *parent_rate); + ret = clk_aic32x4_pll_calc_muldiv(&settings, req->rate, req->best_parent_rate); if (ret < 0) - return 0; + return -EINVAL;
- return clk_aic32x4_pll_calc_rate(&settings, *parent_rate); + rate = clk_aic32x4_pll_calc_rate(&settings, req->best_parent_rate); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; }
static int clk_aic32x4_pll_set_rate(struct clk_hw *hw, @@ -267,7 +272,7 @@ static const struct clk_ops aic32x4_pll_ops = { .unprepare = clk_aic32x4_pll_unprepare, .is_prepared = clk_aic32x4_pll_is_prepared, .recalc_rate = clk_aic32x4_pll_recalc_rate, - .round_rate = clk_aic32x4_pll_round_rate, + .determine_rate = clk_aic32x4_pll_determine_rate, .set_rate = clk_aic32x4_pll_set_rate, .set_parent = clk_aic32x4_pll_set_parent, .get_parent = clk_aic32x4_pll_get_parent,
On Tue, Apr 04, 2023 at 12:11:53PM +0200, Maxime Ripard wrote:
The driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Similar comments to the other patch, I'm pretty sure this is just surprising design on the part of the clock API and we should just allow reparenting.
The tlv320aic32x4 divider clocks 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 driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- sound/soc/codecs/tlv320aic32x4-clk.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/tlv320aic32x4-clk.c b/sound/soc/codecs/tlv320aic32x4-clk.c index d8b8ea3eaa12..707c9951fac0 100644 --- a/sound/soc/codecs/tlv320aic32x4-clk.c +++ b/sound/soc/codecs/tlv320aic32x4-clk.c @@ -333,16 +333,17 @@ static int clk_aic32x4_div_set_rate(struct clk_hw *hw, unsigned long rate, AIC32X4_DIV_MASK, divisor); }
-static long clk_aic32x4_div_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int clk_aic32x4_div_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { unsigned long divisor;
- divisor = DIV_ROUND_UP(*parent_rate, rate); + divisor = DIV_ROUND_UP(req->best_parent_rate, req->rate); if (divisor > 128) return -EINVAL;
- return DIV_ROUND_UP(*parent_rate, divisor); + req->rate = DIV_ROUND_UP(req->best_parent_rate, divisor); + return 0; }
static unsigned long clk_aic32x4_div_recalc_rate(struct clk_hw *hw, @@ -361,7 +362,7 @@ static const struct clk_ops aic32x4_div_ops = { .prepare = clk_aic32x4_div_prepare, .unprepare = clk_aic32x4_div_unprepare, .set_rate = clk_aic32x4_div_set_rate, - .round_rate = clk_aic32x4_div_round_rate, + .determine_rate = clk_aic32x4_div_determine_rate, .recalc_rate = clk_aic32x4_div_recalc_rate, };
@@ -389,7 +390,7 @@ static const struct clk_ops aic32x4_bdiv_ops = { .set_parent = clk_aic32x4_bdiv_set_parent, .get_parent = clk_aic32x4_bdiv_get_parent, .set_rate = clk_aic32x4_div_set_rate, - .round_rate = clk_aic32x4_div_round_rate, + .determine_rate = clk_aic32x4_div_determine_rate, .recalc_rate = clk_aic32x4_div_recalc_rate, };
On Tue, Apr 04, 2023 at 12:11:54PM +0200, Maxime Ripard wrote:
The driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent.
However, It's hard to tell whether it's been done on purpose or not.
Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on.
Similar comments to the other patch, I'm pretty sure this is just surprising design on the part of the clock API and we should just allow reparenting.
The determine_rate hook allows to select the proper parent and its rate for a given clock configuration. On another hand, set_parent is there to change the parent of a mux.
Some clocks provide a set_parent hook but don't implement determine_rate. In such a case, set_parent is pretty much useless since the clock framework will always assume the current parent is to be used, and we will thus never change it.
This situation can be solved in two ways: - either we don't need to change the parent, and we thus shouldn't implement set_parent; - or we don't want to change the parent, in this case we should set CLK_SET_RATE_NO_REPARENT; - or we're missing a determine_rate implementation.
The latter is probably just an oversight from the driver's author, and we should thus raise their awareness about the fact that the current state of the driver is confusing.
All the drivers in-tree have been converted by now, so let's prevent any clock with set_parent but without determine_rate to register so that it can't sneak in again in the future.
Reviewed-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/clk/clk.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index f9fc8730ed17..4c1f28fb8c1f 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3746,6 +3746,13 @@ static int __clk_core_init(struct clk_core *core) goto out; }
+ if (core->ops->set_parent && !core->ops->determine_rate) { + pr_err("%s: %s must implement .set_parent & .determine_rate\n", + __func__, core->name); + ret = -EINVAL; + goto out; + } + if (core->num_parents > 1 && !core->ops->get_parent) { pr_err("%s: %s must implement .get_parent as it has multi parents\n", __func__, core->name);
Quoting Maxime Ripard (2023-04-04 03:10:50)
Hi,
This is a follow-up to a previous series that was printing a warning when a mux has a set_parent implementation but is missing determine_rate().
The rationale is that set_parent() is very likely to be useful when changing the rate, but it's determine_rate() that takes the parenting decision. If we're missing it, then the current parent is always going to be used, and thus set_parent() will not be used. The only exception being a direct call to clk_set_parent(), but those are fairly rare compared to clk_set_rate().
Stephen then asked to promote the warning to an error, and to fix up all the muxes that are in that situation first. So here it is :)
Thanks for resending.
I was thinking that we apply this patch first and then set determine_rate clk_ops without setting the clk flag. The function name is up for debate.
---8<--- diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 27c30a533759..057dd3ca8920 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -594,45 +594,57 @@ clk_core_forward_rate_req(struct clk_core *core, req->max_rate = old_req->max_rate; }
-int clk_mux_determine_rate_flags(struct clk_hw *hw, - struct clk_rate_request *req, - unsigned long flags) +static int +clk_core_determine_rate_noreparent(struct clk_core *core, + struct clk_rate_request *req) { - struct clk_core *core = hw->core, *parent, *best_parent = NULL; - int i, num_parents, ret; + struct clk_core *parent; + int ret; unsigned long best = 0;
- /* if NO_REPARENT flag set, pass through to current parent */ - if (core->flags & CLK_SET_RATE_NO_REPARENT) { - parent = core->parent; - if (core->flags & CLK_SET_RATE_PARENT) { - struct clk_rate_request parent_req; - - if (!parent) { - req->rate = 0; - return 0; - } + parent = core->parent; + if (core->flags & CLK_SET_RATE_PARENT) { + struct clk_rate_request parent_req;
- clk_core_forward_rate_req(core, req, parent, &parent_req, req->rate); + if (!parent) { + req->rate = 0; + return 0; + }
- trace_clk_rate_request_start(&parent_req); + clk_core_forward_rate_req(core, req, parent, &parent_req, req->rate);
- ret = clk_core_round_rate_nolock(parent, &parent_req); - if (ret) - return ret; + trace_clk_rate_request_start(&parent_req);
- trace_clk_rate_request_done(&parent_req); + ret = clk_core_round_rate_nolock(parent, &parent_req); + if (ret) + return ret;
- best = parent_req.rate; - } else if (parent) { - best = clk_core_get_rate_nolock(parent); - } else { - best = clk_core_get_rate_nolock(core); - } + trace_clk_rate_request_done(&parent_req);
- goto out; + best = parent_req.rate; + } else if (parent) { + best = clk_core_get_rate_nolock(parent); + } else { + best = clk_core_get_rate_nolock(core); }
+ req->rate = best; + + return 0; +} + +int clk_mux_determine_rate_flags(struct clk_hw *hw, + struct clk_rate_request *req, + unsigned long flags) +{ + struct clk_core *core = hw->core, *parent, *best_parent = NULL; + int i, num_parents, ret; + unsigned long best = 0; + + /* if NO_REPARENT flag set, pass through to current parent */ + if (core->flags & CLK_SET_RATE_NO_REPARENT) + return clk_core_determine_rate_noreparent(core, req); + /* find the parent that can provide the fastest rate <= rate */ num_parents = core->num_parents; for (i = 0; i < num_parents; i++) { @@ -670,9 +682,7 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw, if (!best_parent) return -EINVAL;
-out: - if (best_parent) - req->best_parent_hw = best_parent->hw; + req->best_parent_hw = best_parent->hw; req->best_parent_rate = best; req->rate = best;
@@ -772,6 +782,24 @@ int __clk_mux_determine_rate_closest(struct clk_hw *hw, } EXPORT_SYMBOL_GPL(__clk_mux_determine_rate_closest);
+/** + * clk_hw_determine_rate_noreparent - clk_ops::determine_rate implementation for a clk that doesn't reparent + * @hw: clk to determine rate on + * @req: rate request + * + * Helper for finding best parent rate to provide a given frequency. This can + * be used directly as a determine_rate callback (e.g. for a mux), or from a + * more complex clock that may combine a mux with other operations. + * + * Returns: 0 on success, -EERROR value on error + */ +int clk_hw_determine_rate_noreparent(struct clk_hw *hw, + struct clk_rate_request *req) +{ + return clk_core_determine_rate_noreparent(hw->core, req); +} +EXPORT_SYMBOL_GPL(clk_hw_determine_rate_noreparent); + /*** clk api ***/
static void clk_core_rate_unprotect(struct clk_core *core) diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 28ff6f1a6ada..958977231ff7 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -1333,6 +1333,8 @@ int __clk_mux_determine_rate_closest(struct clk_hw *hw, int clk_mux_determine_rate_flags(struct clk_hw *hw, struct clk_rate_request *req, unsigned long flags); +int clk_hw_determine_rate_noreparent(struct clk_hw *hw, + struct clk_rate_request *req); void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent); void clk_hw_get_rate_range(struct clk_hw *hw, unsigned long *min_rate, unsigned long *max_rate);
On Thu, Apr 13, 2023 at 02:44:51PM -0700, Stephen Boyd wrote:
Quoting Maxime Ripard (2023-04-04 03:10:50)
Hi,
This is a follow-up to a previous series that was printing a warning when a mux has a set_parent implementation but is missing determine_rate().
The rationale is that set_parent() is very likely to be useful when changing the rate, but it's determine_rate() that takes the parenting decision. If we're missing it, then the current parent is always going to be used, and thus set_parent() will not be used. The only exception being a direct call to clk_set_parent(), but those are fairly rare compared to clk_set_rate().
Stephen then asked to promote the warning to an error, and to fix up all the muxes that are in that situation first. So here it is :)
Thanks for resending.
I was thinking that we apply this patch first and then set determine_rate clk_ops without setting the clk flag. The function name is up for debate.
Ack, I'll send a new version following your proposal
Maxime
-
Charles Keepax -
Claudiu.Beznea＠microchip.com -
David Lechner -
Dinh Nguyen -
Geert Uytterhoeven -
Linus Walleij -
Mark Brown -
Maxime Ripard -
Miquel Raynal -
Paul Cercueil -
Stephen Boyd