Re: [PATCH v3 00/65] clk: Make determine_rate mandatory for muxes
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 signature.asc Description: PGP signature
Re: [PATCH v3 00/65] clk: Make determine_rate mandatory for muxes
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,
_req, req->rate);
+ if (!parent) {
+ req->rate = 0;
+ return 0;
+ }
- trace_clk_rate_request_start(_req);
+ clk_core_forward_rate_req(core, req, parent, _req,
req->rate);
- ret = clk_core_round_rate_nolock(parent, _req);
- if (ret)
- return ret;
+ trace_clk_rate_request_start(_req);
- trace_clk_rate_request_done(_req);
+ ret = clk_core_round_rate_nolock(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(_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
[PATCH v3 00/65] clk: Make determine_rate mandatory for muxes
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
---
Changes in v3:
- Rebased on top of next-20230404
- Link to v2:
https://lore.kernel.org/r/20221018-clk-range-checks-fixes-v2-0-f6736dec1...@cerno.tech
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-f3ef80518...@cerno.tech
---
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:
