Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH RFC 0/5] clk: let clock claim resources
@ 2019-08-28 10:20 Jerome Brunet
  2019-08-28 10:20 ` [PATCH RFC 1/5] clk: actually call the clock init before any other callback of the clock Jerome Brunet
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Jerome Brunet @ 2019-08-28 10:20 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd; +Cc: Jerome Brunet, Kevin Hilman, linux-clk

This patchset is a follow up on this pinky swear [0].
Its purpose is:
 * Clarify the acceptable use of clk_ops init() callback
 * Let the init() callback return an error code in case anything
   fail.
 * Add the terminate() counter part of of init() to release the
   resources which may have been claimed in init()
 * Add a per-clock placeholder for clock runtime data in the clock
   core. This may be useful to init() and save/restore_context()

It was initially suggested to rename these callbacks register/deregister().
But, 'register' is reserved word of C ... :P

In the end, after discussing with Mike, I decided to keep the name "init".
It does not feel that important to change this. I really don't mind
changing this if you feel differently and have a suggestion.

The last patch in this series is just an example of how the above can be
used.

This is sent as an RFC to get the discussion going without bothering too
many people.

In the final series, Patch 2 and 3 will probably be squashed and series
sent to a wider audience.

[0]: https://lkml.kernel.org/r/CAEG3pNB-143Pr_xCTPj=tURhpiTiJqi61xfDGDVdU7zG5H-2tA@mail.gmail.com

Jerome Brunet (5):
  clk: actually call the clock init before any other callback of the
    clock
  clk: let init callback return an error code
  clk: add terminate callback to clk_ops
  clk: add placeholder for clock internal data
  clk: meson: sclk-div: use runtime data

 drivers/clk/clk.c                     | 51 ++++++++++++++-----
 drivers/clk/meson/clk-mpll.c          |  4 +-
 drivers/clk/meson/clk-phase.c         |  4 +-
 drivers/clk/meson/clk-pll.c           |  4 +-
 drivers/clk/meson/sclk-div.c          | 72 +++++++++++++++++++--------
 drivers/clk/meson/sclk-div.h          |  2 -
 drivers/clk/microchip/clk-core.c      |  8 ++-
 drivers/clk/mmp/clk-frac.c            |  4 +-
 drivers/clk/mmp/clk-mix.c             |  4 +-
 drivers/clk/qcom/clk-hfpll.c          |  6 ++-
 drivers/clk/rockchip/clk-pll.c        | 28 +++++++----
 drivers/clk/ti/clock.h                |  2 +-
 drivers/clk/ti/clockdomain.c          |  8 +--
 drivers/net/phy/mdio-mux-meson-g12a.c |  4 +-
 include/linux/clk-provider.h          | 15 ++++--
 15 files changed, 154 insertions(+), 62 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH RFC 1/5] clk: actually call the clock init before any other callback of the clock
  2019-08-28 10:20 [PATCH RFC 0/5] clk: let clock claim resources Jerome Brunet
@ 2019-08-28 10:20 ` Jerome Brunet
  2019-08-28 10:20 ` [PATCH RFC 2/5] clk: let init callback return an error code Jerome Brunet
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Jerome Brunet @ 2019-08-28 10:20 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd; +Cc: Jerome Brunet, Kevin Hilman, linux-clk

 __clk_init_parent() will call the .get_parent() callback of the clock
 so .init() must run before.

Fixes: 541debae0adf ("clk: call the clock init() callback before any other ops callback")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2ba52b8dafcc..4ae25c1c1824 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3306,6 +3306,21 @@ static int __clk_core_init(struct clk_core *core)
 		goto out;
 	}
 
+	/*
+	 * optional platform-specific magic
+	 *
+	 * The .init callback is not used by any of the basic clock types, but
+	 * exists for weird hardware that must perform initialization magic.
+	 * Please consider other ways of solving initialization problems before
+	 * using this callback, as its use is discouraged.
+	 *
+	 * If it exist, this callback should called before any other callback of
+	 * the clock
+	 */
+	if (core->ops->init)
+		core->ops->init(core->hw);
+
+
 	core->parent = __clk_init_parent(core);
 
 	/*
@@ -3330,17 +3345,6 @@ static int __clk_core_init(struct clk_core *core)
 		core->orphan = true;
 	}
 
-	/*
-	 * optional platform-specific magic
-	 *
-	 * The .init callback is not used by any of the basic clock types, but
-	 * exists for weird hardware that must perform initialization magic.
-	 * Please consider other ways of solving initialization problems before
-	 * using this callback, as its use is discouraged.
-	 */
-	if (core->ops->init)
-		core->ops->init(core->hw);
-
 	/*
 	 * Set clk's accuracy.  The preferred method is to use
 	 * .recalc_accuracy. For simple clocks and lazy developers the default
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH RFC 2/5] clk: let init callback return an error code
  2019-08-28 10:20 [PATCH RFC 0/5] clk: let clock claim resources Jerome Brunet
  2019-08-28 10:20 ` [PATCH RFC 1/5] clk: actually call the clock init before any other callback of the clock Jerome Brunet
@ 2019-08-28 10:20 ` Jerome Brunet
  2019-08-28 10:20 ` [PATCH RFC 3/5] clk: add terminate callback to clk_ops Jerome Brunet
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Jerome Brunet @ 2019-08-28 10:20 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd; +Cc: Jerome Brunet, Kevin Hilman, linux-clk

If the init callback is allowed to request resources, it needs a return
value to report the outcome of such a request.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c                     | 17 ++++++++++------
 drivers/clk/meson/clk-mpll.c          |  4 +++-
 drivers/clk/meson/clk-phase.c         |  4 +++-
 drivers/clk/meson/clk-pll.c           |  4 +++-
 drivers/clk/meson/sclk-div.c          |  4 +++-
 drivers/clk/microchip/clk-core.c      |  8 ++++++--
 drivers/clk/mmp/clk-frac.c            |  4 +++-
 drivers/clk/mmp/clk-mix.c             |  4 +++-
 drivers/clk/qcom/clk-hfpll.c          |  6 ++++--
 drivers/clk/rockchip/clk-pll.c        | 28 ++++++++++++++++-----------
 drivers/clk/ti/clock.h                |  2 +-
 drivers/clk/ti/clockdomain.c          |  8 +++++---
 drivers/net/phy/mdio-mux-meson-g12a.c |  4 +++-
 include/linux/clk-provider.h          | 10 +++++++---
 14 files changed, 72 insertions(+), 35 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4ae25c1c1824..f1681555abdf 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3310,16 +3310,21 @@ static int __clk_core_init(struct clk_core *core)
 	 * optional platform-specific magic
 	 *
 	 * The .init callback is not used by any of the basic clock types, but
-	 * exists for weird hardware that must perform initialization magic.
-	 * Please consider other ways of solving initialization problems before
-	 * using this callback, as its use is discouraged.
+	 * exists for weird hardware that must perform initialization magic for
+	 * CCF to get an accurate view of clock for any other callbacks. It may
+	 * also be used needs to perform dynamic allocations. Such allocation
+	 * must be freed in the terminate() callback.
+	 * This callback shall not be used to initialize the parameters state,
+	 * such as rate, parent, etc ...
 	 *
 	 * If it exist, this callback should called before any other callback of
 	 * the clock
 	 */
-	if (core->ops->init)
-		core->ops->init(core->hw);
-
+	if (core->ops->init) {
+		ret = core->ops->init(core->hw);
+		if (ret)
+			goto out;
+	}
 
 	core->parent = __clk_init_parent(core);
 
diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
index 2d39a8bc367c..fc9df4860872 100644
--- a/drivers/clk/meson/clk-mpll.c
+++ b/drivers/clk/meson/clk-mpll.c
@@ -129,7 +129,7 @@ static int mpll_set_rate(struct clk_hw *hw,
 	return 0;
 }
 
-static void mpll_init(struct clk_hw *hw)
+static int mpll_init(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_mpll_data *mpll = meson_clk_mpll_data(clk);
@@ -151,6 +151,8 @@ static void mpll_init(struct clk_hw *hw)
 	/* Set the magic misc bit if required */
 	if (MESON_PARM_APPLICABLE(&mpll->misc))
 		meson_parm_write(clk->map, &mpll->misc, 1);
+
+	return 0;
 }
 
 const struct clk_ops meson_clk_mpll_ro_ops = {
diff --git a/drivers/clk/meson/clk-phase.c b/drivers/clk/meson/clk-phase.c
index 80c3ada193a4..fe22e171121a 100644
--- a/drivers/clk/meson/clk-phase.c
+++ b/drivers/clk/meson/clk-phase.c
@@ -78,7 +78,7 @@ meson_clk_triphase_data(struct clk_regmap *clk)
 	return (struct meson_clk_triphase_data *)clk->data;
 }
 
-static void meson_clk_triphase_sync(struct clk_hw *hw)
+static int meson_clk_triphase_sync(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_triphase_data *tph = meson_clk_triphase_data(clk);
@@ -88,6 +88,8 @@ static void meson_clk_triphase_sync(struct clk_hw *hw)
 	val = meson_parm_read(clk->map, &tph->ph0);
 	meson_parm_write(clk->map, &tph->ph1, val);
 	meson_parm_write(clk->map, &tph->ph2, val);
+
+	return 0;
 }
 
 static int meson_clk_triphase_get_phase(struct clk_hw *hw)
diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index ddb1e5634739..489092dde3a6 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -277,7 +277,7 @@ static int meson_clk_pll_wait_lock(struct clk_hw *hw)
 	return -ETIMEDOUT;
 }
 
-static void meson_clk_pll_init(struct clk_hw *hw)
+static int meson_clk_pll_init(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
@@ -288,6 +288,8 @@ static void meson_clk_pll_init(struct clk_hw *hw)
 				       pll->init_count);
 		meson_parm_write(clk->map, &pll->rst, 0);
 	}
+
+	return 0;
 }
 
 static int meson_clk_pll_is_enabled(struct clk_hw *hw)
diff --git a/drivers/clk/meson/sclk-div.c b/drivers/clk/meson/sclk-div.c
index 3acf03780221..76d31c0a3342 100644
--- a/drivers/clk/meson/sclk-div.c
+++ b/drivers/clk/meson/sclk-div.c
@@ -216,7 +216,7 @@ static int sclk_div_is_enabled(struct clk_hw *hw)
 	return 0;
 }
 
-static void sclk_div_init(struct clk_hw *hw)
+static int sclk_div_init(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
@@ -231,6 +231,8 @@ static void sclk_div_init(struct clk_hw *hw)
 		sclk->cached_div = val + 1;
 
 	sclk_div_get_duty_cycle(hw, &sclk->cached_duty);
+
+	return 0;
 }
 
 const struct clk_ops meson_sclk_div_ops = {
diff --git a/drivers/clk/microchip/clk-core.c b/drivers/clk/microchip/clk-core.c
index 567755d6f844..1b4f023cdc8b 100644
--- a/drivers/clk/microchip/clk-core.c
+++ b/drivers/clk/microchip/clk-core.c
@@ -266,10 +266,12 @@ static void roclk_disable(struct clk_hw *hw)
 	writel(REFO_ON | REFO_OE, PIC32_CLR(refo->ctrl_reg));
 }
 
-static void roclk_init(struct clk_hw *hw)
+static int roclk_init(struct clk_hw *hw)
 {
 	/* initialize clock in disabled state */
 	roclk_disable(hw);
+
+	return 0;
 }
 
 static u8 roclk_get_parent(struct clk_hw *hw)
@@ -880,7 +882,7 @@ static int sclk_set_parent(struct clk_hw *hw, u8 index)
 	return err;
 }
 
-static void sclk_init(struct clk_hw *hw)
+static int sclk_init(struct clk_hw *hw)
 {
 	struct pic32_sys_clk *sclk = clkhw_to_sys_clk(hw);
 	unsigned long flags;
@@ -899,6 +901,8 @@ static void sclk_init(struct clk_hw *hw)
 		writel(v, sclk->slew_reg);
 		spin_unlock_irqrestore(&sclk->core->reg_lock, flags);
 	}
+
+	return 0;
 }
 
 /* sclk with post-divider */
diff --git a/drivers/clk/mmp/clk-frac.c b/drivers/clk/mmp/clk-frac.c
index 90bf181f191a..fabc09aca6c4 100644
--- a/drivers/clk/mmp/clk-frac.c
+++ b/drivers/clk/mmp/clk-frac.c
@@ -109,7 +109,7 @@ static int clk_factor_set_rate(struct clk_hw *hw, unsigned long drate,
 	return 0;
 }
 
-static void clk_factor_init(struct clk_hw *hw)
+static int clk_factor_init(struct clk_hw *hw)
 {
 	struct mmp_clk_factor *factor = to_clk_factor(hw);
 	struct mmp_clk_factor_masks *masks = factor->masks;
@@ -146,6 +146,8 @@ static void clk_factor_init(struct clk_hw *hw)
 
 	if (factor->lock)
 		spin_unlock_irqrestore(factor->lock, flags);
+
+	return 0;
 }
 
 static const struct clk_ops clk_factor_ops = {
diff --git a/drivers/clk/mmp/clk-mix.c b/drivers/clk/mmp/clk-mix.c
index 90814b2613c0..d2cd36c54474 100644
--- a/drivers/clk/mmp/clk-mix.c
+++ b/drivers/clk/mmp/clk-mix.c
@@ -419,12 +419,14 @@ static int mmp_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	}
 }
 
-static void mmp_clk_mix_init(struct clk_hw *hw)
+static int mmp_clk_mix_init(struct clk_hw *hw)
 {
 	struct mmp_clk_mix *mix = to_clk_mix(hw);
 
 	if (mix->table)
 		_filter_clk_table(mix, mix->table, mix->table_size);
+
+	return 0;
 }
 
 const struct clk_ops mmp_clk_mix_ops = {
diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
index 3c04805f2a55..e847d586a73a 100644
--- a/drivers/clk/qcom/clk-hfpll.c
+++ b/drivers/clk/qcom/clk-hfpll.c
@@ -196,7 +196,7 @@ static unsigned long clk_hfpll_recalc_rate(struct clk_hw *hw,
 	return l_val * parent_rate;
 }
 
-static void clk_hfpll_init(struct clk_hw *hw)
+static int clk_hfpll_init(struct clk_hw *hw)
 {
 	struct clk_hfpll *h = to_clk_hfpll(hw);
 	struct hfpll_data const *hd = h->d;
@@ -206,7 +206,7 @@ static void clk_hfpll_init(struct clk_hw *hw)
 	regmap_read(regmap, hd->mode_reg, &mode);
 	if (mode != (PLL_BYPASSNL | PLL_RESET_N | PLL_OUTCTRL)) {
 		__clk_hfpll_init_once(hw);
-		return;
+		return 0;
 	}
 
 	if (hd->status_reg) {
@@ -218,6 +218,8 @@ static void clk_hfpll_init(struct clk_hw *hw)
 			__clk_hfpll_init_once(hw);
 		}
 	}
+
+	return 0;
 }
 
 static int hfpll_is_enabled(struct clk_hw *hw)
diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 198417d56300..10560d963baf 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -282,7 +282,7 @@ static int rockchip_rk3036_pll_is_enabled(struct clk_hw *hw)
 	return !(pllcon & RK3036_PLLCON1_PWRDOWN);
 }
 
-static void rockchip_rk3036_pll_init(struct clk_hw *hw)
+static int rockchip_rk3036_pll_init(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
 	const struct rockchip_pll_rate_table *rate;
@@ -290,14 +290,14 @@ static void rockchip_rk3036_pll_init(struct clk_hw *hw)
 	unsigned long drate;
 
 	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
-		return;
+		return 0;
 
 	drate = clk_hw_get_rate(hw);
 	rate = rockchip_get_pll_settings(pll, drate);
 
 	/* when no rate setting for the current rate, rely on clk_set_rate */
 	if (!rate)
-		return;
+		return 0;
 
 	rockchip_rk3036_pll_get_params(pll, &cur);
 
@@ -319,13 +319,15 @@ static void rockchip_rk3036_pll_init(struct clk_hw *hw)
 		if (!parent) {
 			pr_warn("%s: parent of %s not available\n",
 				__func__, __clk_get_name(hw->clk));
-			return;
+			return 0;
 		}
 
 		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
 			 __func__, __clk_get_name(hw->clk));
 		rockchip_rk3036_pll_set_params(pll, rate);
 	}
+
+	return 0;
 }
 
 static const struct clk_ops rockchip_rk3036_pll_clk_norate_ops = {
@@ -515,7 +517,7 @@ static int rockchip_rk3066_pll_is_enabled(struct clk_hw *hw)
 	return !(pllcon & RK3066_PLLCON3_PWRDOWN);
 }
 
-static void rockchip_rk3066_pll_init(struct clk_hw *hw)
+static int rockchip_rk3066_pll_init(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
 	const struct rockchip_pll_rate_table *rate;
@@ -523,14 +525,14 @@ static void rockchip_rk3066_pll_init(struct clk_hw *hw)
 	unsigned long drate;
 
 	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
-		return;
+		return 0;
 
 	drate = clk_hw_get_rate(hw);
 	rate = rockchip_get_pll_settings(pll, drate);
 
 	/* when no rate setting for the current rate, rely on clk_set_rate */
 	if (!rate)
-		return;
+		return 0;
 
 	rockchip_rk3066_pll_get_params(pll, &cur);
 
@@ -543,6 +545,8 @@ static void rockchip_rk3066_pll_init(struct clk_hw *hw)
 			 __func__, clk_hw_get_name(hw));
 		rockchip_rk3066_pll_set_params(pll, rate);
 	}
+
+	return 0;
 }
 
 static const struct clk_ops rockchip_rk3066_pll_clk_norate_ops = {
@@ -761,7 +765,7 @@ static int rockchip_rk3399_pll_is_enabled(struct clk_hw *hw)
 	return !(pllcon & RK3399_PLLCON3_PWRDOWN);
 }
 
-static void rockchip_rk3399_pll_init(struct clk_hw *hw)
+static int rockchip_rk3399_pll_init(struct clk_hw *hw)
 {
 	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
 	const struct rockchip_pll_rate_table *rate;
@@ -769,14 +773,14 @@ static void rockchip_rk3399_pll_init(struct clk_hw *hw)
 	unsigned long drate;
 
 	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
-		return;
+		return 0;
 
 	drate = clk_hw_get_rate(hw);
 	rate = rockchip_get_pll_settings(pll, drate);
 
 	/* when no rate setting for the current rate, rely on clk_set_rate */
 	if (!rate)
-		return;
+		return 0;
 
 	rockchip_rk3399_pll_get_params(pll, &cur);
 
@@ -798,13 +802,15 @@ static void rockchip_rk3399_pll_init(struct clk_hw *hw)
 		if (!parent) {
 			pr_warn("%s: parent of %s not available\n",
 				__func__, __clk_get_name(hw->clk));
-			return;
+			return 0;
 		}
 
 		pr_debug("%s: pll %s: rate params do not match rate table, adjusting\n",
 			 __func__, __clk_get_name(hw->clk));
 		rockchip_rk3399_pll_set_params(pll, rate);
 	}
+
+	return 0;
 }
 
 static const struct clk_ops rockchip_rk3399_pll_clk_norate_ops = {
diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h
index e4b8392ff63c..adef9c16e43b 100644
--- a/drivers/clk/ti/clock.h
+++ b/drivers/clk/ti/clock.h
@@ -252,7 +252,7 @@ extern const struct clk_ops omap_gate_clk_ops;
 
 extern struct ti_clk_features ti_clk_features;
 
-void omap2_init_clk_clkdm(struct clk_hw *hw);
+int omap2_init_clk_clkdm(struct clk_hw *hw);
 int omap2_clkops_enable_clkdm(struct clk_hw *hw);
 void omap2_clkops_disable_clkdm(struct clk_hw *hw);
 
diff --git a/drivers/clk/ti/clockdomain.c b/drivers/clk/ti/clockdomain.c
index 423a99b9f10c..ee56306f79d5 100644
--- a/drivers/clk/ti/clockdomain.c
+++ b/drivers/clk/ti/clockdomain.c
@@ -101,16 +101,16 @@ void omap2_clkops_disable_clkdm(struct clk_hw *hw)
  *
  * Convert a clockdomain name stored in a struct clk 'clk' into a
  * clockdomain pointer, and save it into the struct clk.  Intended to be
- * called during clk_register().  No return value.
+ * called during clk_register(). Returns 0 on success, -EERROR otherwise.
  */
-void omap2_init_clk_clkdm(struct clk_hw *hw)
+int omap2_init_clk_clkdm(struct clk_hw *hw)
 {
 	struct clk_hw_omap *clk = to_clk_hw_omap(hw);
 	struct clockdomain *clkdm;
 	const char *clk_name;
 
 	if (!clk->clkdm_name)
-		return;
+		return 0;
 
 	clk_name = __clk_get_name(hw->clk);
 
@@ -123,6 +123,8 @@ void omap2_init_clk_clkdm(struct clk_hw *hw)
 		pr_debug("clock: could not associate clk %s to clkdm %s\n",
 			 clk_name, clk->clkdm_name);
 	}
+
+	return 0;
 }
 
 static void __init of_ti_clockdomain_setup(struct device_node *node)
diff --git a/drivers/net/phy/mdio-mux-meson-g12a.c b/drivers/net/phy/mdio-mux-meson-g12a.c
index 6644762ff2ab..1e04c46ac5c1 100644
--- a/drivers/net/phy/mdio-mux-meson-g12a.c
+++ b/drivers/net/phy/mdio-mux-meson-g12a.c
@@ -123,7 +123,7 @@ static int g12a_ephy_pll_is_enabled(struct clk_hw *hw)
 	return (val & PLL_CTL0_LOCK_DIG) ? 1 : 0;
 }
 
-static void g12a_ephy_pll_init(struct clk_hw *hw)
+static int g12a_ephy_pll_init(struct clk_hw *hw)
 {
 	struct g12a_ephy_pll *pll = g12a_ephy_pll_to_dev(hw);
 
@@ -136,6 +136,8 @@ static void g12a_ephy_pll_init(struct clk_hw *hw)
 	writel(0x20200000, pll->base + ETH_PLL_CTL5);
 	writel(0x0000c002, pll->base + ETH_PLL_CTL6);
 	writel(0x00000023, pll->base + ETH_PLL_CTL7);
+
+	return 0;
 }
 
 static const struct clk_ops g12a_ephy_pll_ops = {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 2fdfe8061363..b82ec4c4ca95 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -190,8 +190,12 @@ struct clk_duty {
  *
  * @init:	Perform platform-specific initialization magic.
  *		This is not not used by any of the basic clock types.
- *		Please consider other ways of solving initialization problems
- *		before using this callback, as its use is discouraged.
+ *		This callback exist for HW which needs to perform some
+ *		initialisation magic for CCF to get an accurate view of the
+ *		clock. It may also be used dynamic resource allocation is
+ *		required. It shall not used to deal with clock parameters,
+ *		such as rate or parents.
+ *		Returns 0 on success, -EERROR otherwise.
  *
  * @debug_init:	Set up type-specific debugfs entries for this clock.  This
  *		is called once, after the debugfs directory entry for this
@@ -243,7 +247,7 @@ struct clk_ops {
 					  struct clk_duty *duty);
 	int		(*set_duty_cycle)(struct clk_hw *hw,
 					  struct clk_duty *duty);
-	void		(*init)(struct clk_hw *hw);
+	int		(*init)(struct clk_hw *hw);
 	void		(*debug_init)(struct clk_hw *hw, struct dentry *dentry);
 };
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH RFC 3/5] clk: add terminate callback to clk_ops
  2019-08-28 10:20 [PATCH RFC 0/5] clk: let clock claim resources Jerome Brunet
  2019-08-28 10:20 ` [PATCH RFC 1/5] clk: actually call the clock init before any other callback of the clock Jerome Brunet
  2019-08-28 10:20 ` [PATCH RFC 2/5] clk: let init callback return an error code Jerome Brunet
@ 2019-08-28 10:20 ` Jerome Brunet
  2019-08-28 10:20 ` [PATCH RFC 4/5] clk: add placeholder for clock internal data Jerome Brunet
  2019-08-28 10:20 ` [PATCH RFC 5/5] clk: meson: sclk-div: use runtime data Jerome Brunet
  4 siblings, 0 replies; 11+ messages in thread
From: Jerome Brunet @ 2019-08-28 10:20 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd; +Cc: Jerome Brunet, Kevin Hilman, linux-clk

Add a terminate callback to the clk_ops to release the resources
claimed in .init()

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c            | 7 ++++++-
 include/linux/clk-provider.h | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f1681555abdf..c703aa35ca37 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3828,6 +3828,7 @@ static const struct clk_ops clk_nodrv_ops = {
 void clk_unregister(struct clk *clk)
 {
 	unsigned long flags;
+	const struct clk_ops *ops;
 
 	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 		return;
@@ -3836,7 +3837,8 @@ void clk_unregister(struct clk *clk)
 
 	clk_prepare_lock();
 
-	if (clk->core->ops == &clk_nodrv_ops) {
+	ops = clk->core->ops;
+	if (ops == &clk_nodrv_ops) {
 		pr_err("%s: unregistered clock: %s\n", __func__,
 		       clk->core->name);
 		goto unlock;
@@ -3849,6 +3851,9 @@ void clk_unregister(struct clk *clk)
 	clk->core->ops = &clk_nodrv_ops;
 	clk_enable_unlock(flags);
 
+	if (ops->terminate)
+		ops->terminate(clk->core->hw);
+
 	if (!hlist_empty(&clk->core->children)) {
 		struct clk_core *child;
 		struct hlist_node *t;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index b82ec4c4ca95..5a5a64785923 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -197,6 +197,8 @@ struct clk_duty {
  *		such as rate or parents.
  *		Returns 0 on success, -EERROR otherwise.
  *
+ * @terminate:  Free any resource allocated by init.
+ *
  * @debug_init:	Set up type-specific debugfs entries for this clock.  This
  *		is called once, after the debugfs directory entry for this
  *		clock has been created.  The dentry pointer representing that
@@ -248,6 +250,7 @@ struct clk_ops {
 	int		(*set_duty_cycle)(struct clk_hw *hw,
 					  struct clk_duty *duty);
 	int		(*init)(struct clk_hw *hw);
+	void		(*terminate)(struct clk_hw *hw);
 	void		(*debug_init)(struct clk_hw *hw, struct dentry *dentry);
 };
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH RFC 4/5] clk: add placeholder for clock internal data
  2019-08-28 10:20 [PATCH RFC 0/5] clk: let clock claim resources Jerome Brunet
                   ` (2 preceding siblings ...)
  2019-08-28 10:20 ` [PATCH RFC 3/5] clk: add terminate callback to clk_ops Jerome Brunet
@ 2019-08-28 10:20 ` Jerome Brunet
  2019-08-28 22:15   ` Stephen Boyd
  2019-08-28 10:20 ` [PATCH RFC 5/5] clk: meson: sclk-div: use runtime data Jerome Brunet
  4 siblings, 1 reply; 11+ messages in thread
From: Jerome Brunet @ 2019-08-28 10:20 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd; +Cc: Jerome Brunet, Kevin Hilman, linux-clk

Add placeholder in clock core to save per clock data.
Such placeholder could use for clock doing memory allocation in .init().
It may also be useful for the save/restore_context() callbacks.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c            | 13 +++++++++++++
 include/linux/clk-provider.h |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c703aa35ca37..aa77a2a98ea4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -83,6 +83,7 @@ struct clk_core {
 	struct hlist_node	debug_node;
 #endif
 	struct kref		ref;
+	void			*priv;
 };
 
 #define CREATE_TRACE_POINTS
@@ -281,6 +282,18 @@ struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw)
 }
 EXPORT_SYMBOL_GPL(clk_hw_get_parent);
 
+void clk_hw_set_data(const struct clk_hw *hw, void *data)
+{
+	hw->core->priv = data;
+}
+EXPORT_SYMBOL_GPL(clk_hw_set_data);
+
+void *clk_hw_get_data(const struct clk_hw *hw)
+{
+	return hw->core->priv;
+}
+EXPORT_SYMBOL_GPL(clk_hw_get_data);
+
 static struct clk_core *__clk_lookup_subtree(const char *name,
 					     struct clk_core *core)
 {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5a5a64785923..e54c165af021 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -821,6 +821,8 @@ static inline struct clk_hw *__clk_get_hw(struct clk *clk)
 	return (struct clk_hw *)clk;
 }
 #endif
+void clk_hw_set_data(const struct clk_hw *hw, void *data);
+void *clk_hw_get_data(const struct clk_hw *hw);
 unsigned int clk_hw_get_num_parents(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw,
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH RFC 5/5] clk: meson: sclk-div: use runtime data
  2019-08-28 10:20 [PATCH RFC 0/5] clk: let clock claim resources Jerome Brunet
                   ` (3 preceding siblings ...)
  2019-08-28 10:20 ` [PATCH RFC 4/5] clk: add placeholder for clock internal data Jerome Brunet
@ 2019-08-28 10:20 ` Jerome Brunet
  4 siblings, 0 replies; 11+ messages in thread
From: Jerome Brunet @ 2019-08-28 10:20 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd; +Cc: Jerome Brunet, Kevin Hilman, linux-clk

Remove the sclk-div runtime data from the clock description structure
and use the per clock placeholder to save the these runtime data.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/meson/sclk-div.c | 68 ++++++++++++++++++++++++++----------
 drivers/clk/meson/sclk-div.h |  2 --
 2 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/meson/sclk-div.c b/drivers/clk/meson/sclk-div.c
index 76d31c0a3342..32d488086437 100644
--- a/drivers/clk/meson/sclk-div.c
+++ b/drivers/clk/meson/sclk-div.c
@@ -18,10 +18,16 @@
 
 #include <linux/clk-provider.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 
 #include "clk-regmap.h"
 #include "sclk-div.h"
 
+struct sclk_div_runtime {
+	unsigned int div;
+	struct clk_duty duty;
+};
+
 static inline struct meson_sclk_div_data *
 meson_sclk_div_data(struct clk_regmap *clk)
 {
@@ -109,11 +115,12 @@ static long sclk_div_round_rate(struct clk_hw *hw, unsigned long rate,
 }
 
 static void sclk_apply_ratio(struct clk_regmap *clk,
-			     struct meson_sclk_div_data *sclk)
+			     struct meson_sclk_div_data *sclk,
+			     struct sclk_div_runtime *runtime)
 {
-	unsigned int hi = DIV_ROUND_CLOSEST(sclk->cached_div *
-					    sclk->cached_duty.num,
-					    sclk->cached_duty.den);
+	unsigned int hi = DIV_ROUND_CLOSEST(runtime->div *
+					    runtime->duty.num,
+					    runtime->duty.den);
 
 	if (hi)
 		hi -= 1;
@@ -126,10 +133,12 @@ static int sclk_div_set_duty_cycle(struct clk_hw *hw,
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
+	struct sclk_div_runtime *runtime =
+		(struct sclk_div_runtime *) clk_hw_get_data(hw);
 
 	if (MESON_PARM_APPLICABLE(&sclk->hi)) {
-		memcpy(&sclk->cached_duty, duty, sizeof(*duty));
-		sclk_apply_ratio(clk, sclk);
+		memcpy(&runtime->duty, duty, sizeof(*duty));
+		sclk_apply_ratio(clk, sclk, runtime);
 	}
 
 	return 0;
@@ -140,6 +149,8 @@ static int sclk_div_get_duty_cycle(struct clk_hw *hw,
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
+	struct sclk_div_runtime *runtime =
+		(struct sclk_div_runtime *) clk_hw_get_data(hw);
 	int hi;
 
 	if (!MESON_PARM_APPLICABLE(&sclk->hi)) {
@@ -150,17 +161,18 @@ static int sclk_div_get_duty_cycle(struct clk_hw *hw,
 
 	hi = meson_parm_read(clk->map, &sclk->hi);
 	duty->num = hi + 1;
-	duty->den = sclk->cached_div;
+	duty->den = runtime->div;
 	return 0;
 }
 
 static void sclk_apply_divider(struct clk_regmap *clk,
-			       struct meson_sclk_div_data *sclk)
+			       struct meson_sclk_div_data *sclk,
+			       struct sclk_div_runtime *runtime)
 {
 	if (MESON_PARM_APPLICABLE(&sclk->hi))
-		sclk_apply_ratio(clk, sclk);
+		sclk_apply_ratio(clk, sclk, runtime);
 
-	meson_parm_write(clk->map, &sclk->div, sclk->cached_div - 1);
+	meson_parm_write(clk->map, &sclk->div, runtime->div - 1);
 }
 
 static int sclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -168,12 +180,14 @@ static int sclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
+	struct sclk_div_runtime *runtime =
+		(struct sclk_div_runtime *) clk_hw_get_data(hw);
 	unsigned long maxdiv = sclk_div_maxdiv(sclk);
 
-	sclk->cached_div = sclk_div_getdiv(hw, rate, prate, maxdiv);
+	runtime->div = sclk_div_getdiv(hw, rate, prate, maxdiv);
 
 	if (clk_hw_is_enabled(hw))
-		sclk_apply_divider(clk, sclk);
+		sclk_apply_divider(clk, sclk, runtime);
 
 	return 0;
 }
@@ -181,18 +195,20 @@ static int sclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
 static unsigned long sclk_div_recalc_rate(struct clk_hw *hw,
 					  unsigned long prate)
 {
-	struct clk_regmap *clk = to_clk_regmap(hw);
-	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
+	struct sclk_div_runtime *runtime =
+		(struct sclk_div_runtime *) clk_hw_get_data(hw);
 
-	return DIV_ROUND_UP_ULL((u64)prate, sclk->cached_div);
+	return DIV_ROUND_UP_ULL((u64)prate, runtime->div);
 }
 
 static int sclk_div_enable(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
+	struct sclk_div_runtime *runtime =
+		(struct sclk_div_runtime *) clk_hw_get_data(hw);
 
-	sclk_apply_divider(clk, sclk);
+	sclk_apply_divider(clk, sclk, runtime);
 
 	return 0;
 }
@@ -220,21 +236,34 @@ static int sclk_div_init(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
+	struct sclk_div_runtime *runtime;
 	unsigned int val;
 
+	runtime = kzalloc(sizeof(*runtime), GFP_KERNEL);
+	if (!runtime)
+		return -ENOMEM;
+
 	val = meson_parm_read(clk->map, &sclk->div);
 
 	/* if the divider is initially disabled, assume max */
 	if (!val)
-		sclk->cached_div = sclk_div_maxdiv(sclk);
+		runtime->div = sclk_div_maxdiv(sclk);
 	else
-		sclk->cached_div = val + 1;
+		runtime->div = val + 1;
 
-	sclk_div_get_duty_cycle(hw, &sclk->cached_duty);
+	sclk_div_get_duty_cycle(hw, &runtime->duty);
 
 	return 0;
 }
 
+static void sclk_div_terminate(struct clk_hw *hw)
+{
+	struct sclk_div_runtime *runtime =
+		(struct sclk_div_runtime *) clk_hw_get_data(hw);
+
+	kfree(runtime);
+}
+
 const struct clk_ops meson_sclk_div_ops = {
 	.recalc_rate	= sclk_div_recalc_rate,
 	.round_rate	= sclk_div_round_rate,
@@ -245,6 +274,7 @@ const struct clk_ops meson_sclk_div_ops = {
 	.get_duty_cycle	= sclk_div_get_duty_cycle,
 	.set_duty_cycle = sclk_div_set_duty_cycle,
 	.init		= sclk_div_init,
+	.terminate	= sclk_div_terminate,
 };
 EXPORT_SYMBOL_GPL(meson_sclk_div_ops);
 
diff --git a/drivers/clk/meson/sclk-div.h b/drivers/clk/meson/sclk-div.h
index b64b2a32005f..d03bbd78f47b 100644
--- a/drivers/clk/meson/sclk-div.h
+++ b/drivers/clk/meson/sclk-div.h
@@ -13,8 +13,6 @@
 struct meson_sclk_div_data {
 	struct parm div;
 	struct parm hi;
-	unsigned int cached_div;
-	struct clk_duty cached_duty;
 };
 
 extern const struct clk_ops meson_sclk_div_ops;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC 4/5] clk: add placeholder for clock internal data
  2019-08-28 10:20 ` [PATCH RFC 4/5] clk: add placeholder for clock internal data Jerome Brunet
@ 2019-08-28 22:15   ` Stephen Boyd
  2019-08-29  7:20     ` Jerome Brunet
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2019-08-28 22:15 UTC (permalink / raw)
  To: Jerome Brunet, Michael Turquette; +Cc: Jerome Brunet, Kevin Hilman, linux-clk

Quoting Jerome Brunet (2019-08-28 03:20:11)
> Add placeholder in clock core to save per clock data.
> Such placeholder could use for clock doing memory allocation in .init().
> It may also be useful for the save/restore_context() callbacks.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/clk.c            | 13 +++++++++++++
>  include/linux/clk-provider.h |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c703aa35ca37..aa77a2a98ea4 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -83,6 +83,7 @@ struct clk_core {
>         struct hlist_node       debug_node;
>  #endif
>         struct kref             ref;
> +       void                    *priv;

Why? We have container structures around clk_hw that can be used to
store data and clk_ops that should know to deref said clk_hw pointer in
some way to access that data.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC 4/5] clk: add placeholder for clock internal data
  2019-08-28 22:15   ` Stephen Boyd
@ 2019-08-29  7:20     ` Jerome Brunet
  2019-08-29 17:17       ` Stephen Boyd
  0 siblings, 1 reply; 11+ messages in thread
From: Jerome Brunet @ 2019-08-29  7:20 UTC (permalink / raw)
  To: Stephen Boyd, Michael Turquette; +Cc: Kevin Hilman, linux-clk

On Wed 28 Aug 2019 at 15:15, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2019-08-28 03:20:11)
>> Add placeholder in clock core to save per clock data.
>> Such placeholder could use for clock doing memory allocation in .init().
>> It may also be useful for the save/restore_context() callbacks.
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/clk/clk.c            | 13 +++++++++++++
>>  include/linux/clk-provider.h |  2 ++
>>  2 files changed, 15 insertions(+)
>> 
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index c703aa35ca37..aa77a2a98ea4 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -83,6 +83,7 @@ struct clk_core {
>>         struct hlist_node       debug_node;
>>  #endif
>>         struct kref             ref;
>> +       void                    *priv;
>
> Why? We have container structures around clk_hw that can be used to
> store data and clk_ops that should know to deref said clk_hw pointer in
> some way to access that data.

This simple addition give a placeholder to each clock instance that is
hosted under the clock core so we know it can only be accesed from this
particular instance. Seems like a better fit for runtime data, such as
saved context.

It gives a way to avoid mixing the clock description and its runtime
data. In the end, It would be nice if the clock description part could
be made const.

This is a (fairly usual) way to do it 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC 4/5] clk: add placeholder for clock internal data
  2019-08-29  7:20     ` Jerome Brunet
@ 2019-08-29 17:17       ` Stephen Boyd
  2019-08-30 14:06         ` Jerome Brunet
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2019-08-29 17:17 UTC (permalink / raw)
  To: Jerome Brunet, Michael Turquette; +Cc: Kevin Hilman, linux-clk

Quoting Jerome Brunet (2019-08-29 00:20:38)
> On Wed 28 Aug 2019 at 15:15, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Jerome Brunet (2019-08-28 03:20:11)
> >> Add placeholder in clock core to save per clock data.
> >> Such placeholder could use for clock doing memory allocation in .init().
> >> It may also be useful for the save/restore_context() callbacks.
> >> 
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >>  drivers/clk/clk.c            | 13 +++++++++++++
> >>  include/linux/clk-provider.h |  2 ++
> >>  2 files changed, 15 insertions(+)
> >> 
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index c703aa35ca37..aa77a2a98ea4 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -83,6 +83,7 @@ struct clk_core {
> >>         struct hlist_node       debug_node;
> >>  #endif
> >>         struct kref             ref;
> >> +       void                    *priv;
> >
> > Why? We have container structures around clk_hw that can be used to
> > store data and clk_ops that should know to deref said clk_hw pointer in
> > some way to access that data.
> 
> This simple addition give a placeholder to each clock instance that is
> hosted under the clock core so we know it can only be accesed from this
> particular instance. Seems like a better fit for runtime data, such as
> saved context.
> 
> It gives a way to avoid mixing the clock description and its runtime
> data. In the end, It would be nice if the clock description part could
> be made const.
> 
> This is a (fairly usual) way to do it 

Maybe you can provide an example or usage? Is that the last patch in
this series? I still don't quite understand why we need this.

I imagine that if you wanted to have a const description part you could
have a container structure around the clk_hw struct

	struct my_clk_foo {
		const struct clk_description *desc;
		<runtime modifiable members>;
		struct clk_hw hw;
	};

Did I miss something?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC 4/5] clk: add placeholder for clock internal data
  2019-08-29 17:17       ` Stephen Boyd
@ 2019-08-30 14:06         ` Jerome Brunet
  2019-09-04 22:52           ` Stephen Boyd
  0 siblings, 1 reply; 11+ messages in thread
From: Jerome Brunet @ 2019-08-30 14:06 UTC (permalink / raw)
  To: Stephen Boyd, Michael Turquette; +Cc: Kevin Hilman, linux-clk

On Thu 29 Aug 2019 at 10:17, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2019-08-29 00:20:38)
>> On Wed 28 Aug 2019 at 15:15, Stephen Boyd <sboyd@kernel.org> wrote:
>> 
>> > Quoting Jerome Brunet (2019-08-28 03:20:11)
>> >> Add placeholder in clock core to save per clock data.
>> >> Such placeholder could use for clock doing memory allocation in .init().
>> >> It may also be useful for the save/restore_context() callbacks.
>> >> 
>> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> >> ---
>> >>  drivers/clk/clk.c            | 13 +++++++++++++
>> >>  include/linux/clk-provider.h |  2 ++
>> >>  2 files changed, 15 insertions(+)
>> >> 
>> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> >> index c703aa35ca37..aa77a2a98ea4 100644
>> >> --- a/drivers/clk/clk.c
>> >> +++ b/drivers/clk/clk.c
>> >> @@ -83,6 +83,7 @@ struct clk_core {
>> >>         struct hlist_node       debug_node;
>> >>  #endif
>> >>         struct kref             ref;
>> >> +       void                    *priv;
>> >
>> > Why? We have container structures around clk_hw that can be used to
>> > store data and clk_ops that should know to deref said clk_hw pointer in
>> > some way to access that data.
>> 
>> This simple addition give a placeholder to each clock instance that is
>> hosted under the clock core so we know it can only be accesed from this
>> particular instance. Seems like a better fit for runtime data, such as
>> saved context.
>> 
>> It gives a way to avoid mixing the clock description and its runtime
>> data. In the end, It would be nice if the clock description part could
>> be made const.
>> 
>> This is a (fairly usual) way to do it 
>
> Maybe you can provide an example or usage? Is that the last patch in
> this series?

Yes, I thought that was in my cover letter

> I still don't quite understand why we need this.
>
> I imagine that if you wanted to have a const description part you could
> have a container structure around the clk_hw struct
>
> 	struct my_clk_foo {
> 		const struct clk_description *desc;
> 		<runtime modifiable members>;
> 		struct clk_hw hw;
> 	};
>
> Did I miss something?

Not really. As with all C code, there are many way to do things.
Your way could work, mine too.

As explained in the previous mail, what I propose guaranteed to specific
to each clock instance. It's a fairly usual and simple way to give a
placeholder to the instance of something.

Things gets complicated with your way if the clock declaration uses
static data and there there multiple instance of it.

In the end that was merely a suggestion, so let's spend to much time on
this. If don't want it, that's fine. I'll just drop it.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC 4/5] clk: add placeholder for clock internal data
  2019-08-30 14:06         ` Jerome Brunet
@ 2019-09-04 22:52           ` Stephen Boyd
  0 siblings, 0 replies; 11+ messages in thread
From: Stephen Boyd @ 2019-09-04 22:52 UTC (permalink / raw)
  To: Jerome Brunet, Michael Turquette; +Cc: Kevin Hilman, linux-clk

Quoting Jerome Brunet (2019-08-30 07:06:29)
> On Thu 29 Aug 2019 at 10:17, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Jerome Brunet (2019-08-29 00:20:38)
> >> On Wed 28 Aug 2019 at 15:15, Stephen Boyd <sboyd@kernel.org> wrote:
> >> 
> >> > Quoting Jerome Brunet (2019-08-28 03:20:11)
> >> >> Add placeholder in clock core to save per clock data.
> >> >> Such placeholder could use for clock doing memory allocation in .init().
> >> >> It may also be useful for the save/restore_context() callbacks.
> >> >> 
> >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> >> ---
> >> >>  drivers/clk/clk.c            | 13 +++++++++++++
> >> >>  include/linux/clk-provider.h |  2 ++
> >> >>  2 files changed, 15 insertions(+)
> >> >> 
> >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> >> index c703aa35ca37..aa77a2a98ea4 100644
> >> >> --- a/drivers/clk/clk.c
> >> >> +++ b/drivers/clk/clk.c
> >> >> @@ -83,6 +83,7 @@ struct clk_core {
> >> >>         struct hlist_node       debug_node;
> >> >>  #endif
> >> >>         struct kref             ref;
> >> >> +       void                    *priv;
> >> >
> >> > Why? We have container structures around clk_hw that can be used to
> >> > store data and clk_ops that should know to deref said clk_hw pointer in
> >> > some way to access that data.
> >> 
> >> This simple addition give a placeholder to each clock instance that is
> >> hosted under the clock core so we know it can only be accesed from this
> >> particular instance. Seems like a better fit for runtime data, such as
> >> saved context.
> >> 
> >> It gives a way to avoid mixing the clock description and its runtime
> >> data. In the end, It would be nice if the clock description part could
> >> be made const.
> >> 
> >> This is a (fairly usual) way to do it 
> >
> > Maybe you can provide an example or usage? Is that the last patch in
> > this series?
> 
> Yes, I thought that was in my cover letter
> 
> > I still don't quite understand why we need this.
> >
> > I imagine that if you wanted to have a const description part you could
> > have a container structure around the clk_hw struct
> >
> >       struct my_clk_foo {
> >               const struct clk_description *desc;
> >               <runtime modifiable members>;
> >               struct clk_hw hw;
> >       };
> >
> > Did I miss something?
> 
> Not really. As with all C code, there are many way to do things.
> Your way could work, mine too.
> 
> As explained in the previous mail, what I propose guaranteed to specific
> to each clock instance. It's a fairly usual and simple way to give a
> placeholder to the instance of something.
> 
> Things gets complicated with your way if the clock declaration uses
> static data and there there multiple instance of it.
> 
> In the end that was merely a suggestion, so let's spend to much time on
> this. If don't want it, that's fine. I'll just drop it.

Ok. I see that this is how the meson clk driver does clk_regmap structs.
I agree that if the same data exists for many clks then having a pointer
to that data instead of duplicating it is good for saving space, but
that sort of detail can still be put into a void *data member of the
wrapper structure(s) and then passed to some function that knows the
type of the data that's the same. It may require writing a few more
functions to unwrap the data member out of the wrapper struct though so
having a data member of the clk_hw or clk_core pointer may be useful to
make "polymorphic" code that just knows that the data member is some
particular type and can operate on that generically without having to
unwrap the container for each different type.

TL;DR is I'm half convinced that this is a good approach to solving that
sort of problem.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-28 10:20 [PATCH RFC 0/5] clk: let clock claim resources Jerome Brunet
2019-08-28 10:20 ` [PATCH RFC 1/5] clk: actually call the clock init before any other callback of the clock Jerome Brunet
2019-08-28 10:20 ` [PATCH RFC 2/5] clk: let init callback return an error code Jerome Brunet
2019-08-28 10:20 ` [PATCH RFC 3/5] clk: add terminate callback to clk_ops Jerome Brunet
2019-08-28 10:20 ` [PATCH RFC 4/5] clk: add placeholder for clock internal data Jerome Brunet
2019-08-28 22:15   ` Stephen Boyd
2019-08-29  7:20     ` Jerome Brunet
2019-08-29 17:17       ` Stephen Boyd
2019-08-30 14:06         ` Jerome Brunet
2019-09-04 22:52           ` Stephen Boyd
2019-08-28 10:20 ` [PATCH RFC 5/5] clk: meson: sclk-div: use runtime data Jerome Brunet

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git