[v3,00/65] clk: Make determine_rate mandatory for muxes

Message

Maxime Ripard April 4, 2023, 10:10 a.m. UTC

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 <[e-mail]>
---
Changes in v3:
- Rebased on top of next-20230404
- Link to v2: https://lore.kernel.org/r/[e-mail]

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/[e-mail]

---
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,

Comments

Stephen Boyd April 13, 2023, 9:44 p.m. UTC | #1

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);

Maxime Ripard April 25, 2023, 2:46 p.m. UTC | #2

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