diff mbox series

[RFC] clk: rework clk_register to use the clk_hw API

Message ID 20200519170733.295100-1-jbrunet@baylibre.com (mailing list archive)
State RFC, archived
Headers show
Series [RFC] clk: rework clk_register to use the clk_hw API | expand

Commit Message

Jerome Brunet May 19, 2020, 5:07 p.m. UTC
This rework the clk_register/unregister functions to use the clk_hw API.
The goal is to pave the way for the removal of the 'clk' member in
struct clk_hw.

This member is used in about 60 drivers, most of them in drivers/clk/.
Some cases will be trivial to remove but some drivers are hacking around
it and will be tougher to deal with.

This change is sent as an RFC because, until there is a clear plan to deal
with drivers above, there is memory penality when using clk_register()
(struct clk is allocated in __clk_hw_register() and clk_register())

Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 110 ++++++++++++++++++++++++++--------------------
 1 file changed, 62 insertions(+), 48 deletions(-)

Comments

Stephen Boyd Nov. 14, 2020, 9:02 p.m. UTC | #1
Quoting Jerome Brunet (2020-05-19 10:07:33)
> This rework the clk_register/unregister functions to use the clk_hw API.
> The goal is to pave the way for the removal of the 'clk' member in
> struct clk_hw.
> 
> This member is used in about 60 drivers, most of them in drivers/clk/.
> Some cases will be trivial to remove but some drivers are hacking around
> it and will be tougher to deal with.
> 
> This change is sent as an RFC because, until there is a clear plan to deal
> with drivers above, there is memory penality when using clk_register()
> (struct clk is allocated in __clk_hw_register() and clk_register())
> 
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Do you plan to resend this? I'm inclined to just apply it and we can
sort out the memory penalty problem later. I'm not even sure what the
penalty is quite honestly. Drivers can call the clk_hw_get_clk() API you
introduced and generate clk pointer so we should be able to fully hide
the clk generated in clk_register() from them and not even allocate it?
Jerome Brunet Nov. 16, 2020, 3:13 p.m. UTC | #2
On Sat 14 Nov 2020 at 22:02, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2020-05-19 10:07:33)
>> This rework the clk_register/unregister functions to use the clk_hw API.
>> The goal is to pave the way for the removal of the 'clk' member in
>> struct clk_hw.
>> 
>> This member is used in about 60 drivers, most of them in drivers/clk/.
>> Some cases will be trivial to remove but some drivers are hacking around
>> it and will be tougher to deal with.
>> 
>> This change is sent as an RFC because, until there is a clear plan to deal
>> with drivers above, there is memory penality when using clk_register()
>> (struct clk is allocated in __clk_hw_register() and clk_register())
>> 
>> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
> Do you plan to resend this? I'm inclined to just apply it and we can
> sort out the memory penalty problem later.

I had forgot about it :)
It does not apply so I'll fix this and resend

Could you publish an immutable branch with the clk_hw_get_clk(),
devm_clk_notifier and the meson stuff you have applied ?

I'd like to base my next PR on it since Kevin submitted a change which
depends on it. It will also help me rebase this change.

> I'm not even sure what the
> penalty is quite honestly. Drivers can call the clk_hw_get_clk() API you
> introduced and generate clk pointer so we should be able to fully hide
> the clk generated in clk_register() from them and not even allocate it?

Eventually, it is the idea and there would no penality anymore.
At the moment, a fair amount of "struct clk_hw" user rely on the ".clk"
member being present, even if it is bad. All those need modification
before we can kill the .clk member.

I was thinking of a progressive approach:
* Migrate clk_register()
* Nicely ask user the direct users ".clk" to update
* Once satisfied, remove ".clk"
Stephen Boyd Nov. 18, 2020, 1:43 a.m. UTC | #3
Quoting Jerome Brunet (2020-11-16 07:13:59)
> 
> On Sat 14 Nov 2020 at 22:02, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Jerome Brunet (2020-05-19 10:07:33)
> >> This rework the clk_register/unregister functions to use the clk_hw API.
> >> The goal is to pave the way for the removal of the 'clk' member in
> >> struct clk_hw.
> >> 
> >> This member is used in about 60 drivers, most of them in drivers/clk/.
> >> Some cases will be trivial to remove but some drivers are hacking around
> >> it and will be tougher to deal with.
> >> 
> >> This change is sent as an RFC because, until there is a clear plan to deal
> >> with drivers above, there is memory penality when using clk_register()
> >> (struct clk is allocated in __clk_hw_register() and clk_register())
> >> 
> >> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >
> > Do you plan to resend this? I'm inclined to just apply it and we can
> > sort out the memory penalty problem later.
> 
> I had forgot about it :)
> It does not apply so I'll fix this and resend

Cool. I forgot too, just found by searching your email :)

> 
> Could you publish an immutable branch with the clk_hw_get_clk(),
> devm_clk_notifier and the meson stuff you have applied ?

Yeah no problem! It's clk-hw branch.

> 
> > I'm not even sure what the
> > penalty is quite honestly. Drivers can call the clk_hw_get_clk() API you
> > introduced and generate clk pointer so we should be able to fully hide
> > the clk generated in clk_register() from them and not even allocate it?
> 
> Eventually, it is the idea and there would no penality anymore.
> At the moment, a fair amount of "struct clk_hw" user rely on the ".clk"
> member being present, even if it is bad. All those need modification
> before we can kill the .clk member.
> 
> I was thinking of a progressive approach:
> * Migrate clk_register()
> * Nicely ask user the direct users ".clk" to update
> * Once satisfied, remove ".clk"
> 

Sounds good to me.
diff mbox series

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d9946e192cbc..d93062673b2c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3736,8 +3736,8 @@  static void clk_core_free_parent_map(struct clk_core *core)
 	kfree(core->parents);
 }
 
-static struct clk *
-__clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
+static int
+__clk_hw_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 {
 	int ret;
 	struct clk_core *core;
@@ -3801,7 +3801,7 @@  __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 
 	ret = __clk_core_init(core);
 	if (!ret)
-		return hw->clk;
+		return 0;
 
 	clk_prepare_lock();
 	clk_core_unlink_consumer(hw->clk);
@@ -3818,7 +3818,7 @@  __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 fail_name:
 	kfree(core);
 fail_out:
-	return ERR_PTR(ret);
+	return ret;
 }
 
 /**
@@ -3843,6 +3843,24 @@  static struct device_node *dev_or_parent_of_node(struct device *dev)
 	return np;
 }
 
+
+/**
+ * clk_hw_register - register a clk_hw and return an error code
+ * @dev: device that is registering this clock
+ * @hw: link to hardware-specific clock data
+ *
+ * clk_hw_register is the primary interface for populating the clock tree with
+ * new clock nodes. It returns an integer equal to zero indicating success or
+ * less than zero indicating failure. Drivers must test for an error code after
+ * calling clk_hw_register().
+ */
+int clk_hw_register(struct device *dev, struct clk_hw *hw)
+{
+	return __clk_hw_register(dev, dev_or_parent_of_node(dev),
+				 hw);
+}
+EXPORT_SYMBOL_GPL(clk_hw_register);
+
 /**
  * clk_register - allocate a new clock, register it and return an opaque cookie
  * @dev: device that is registering this clock
@@ -3858,26 +3876,19 @@  static struct device_node *dev_or_parent_of_node(struct device *dev)
  */
 struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 {
-	return __clk_register(dev, dev_or_parent_of_node(dev), hw);
-}
-EXPORT_SYMBOL_GPL(clk_register);
+	struct clk *clk;
+	int ret = clk_hw_register(dev, hw);
 
-/**
- * clk_hw_register - register a clk_hw and return an error code
- * @dev: device that is registering this clock
- * @hw: link to hardware-specific clock data
- *
- * clk_hw_register is the primary interface for populating the clock tree with
- * new clock nodes. It returns an integer equal to zero indicating success or
- * less than zero indicating failure. Drivers must test for an error code after
- * calling clk_hw_register().
- */
-int clk_hw_register(struct device *dev, struct clk_hw *hw)
-{
-	return PTR_ERR_OR_ZERO(__clk_register(dev, dev_or_parent_of_node(dev),
-			       hw));
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	clk = clk_hw_get_clk(hw);
+	if (IS_ERR_OR_NULL(clk))
+		clk_hw_unregister(hw);
+
+	return clk;
 }
-EXPORT_SYMBOL_GPL(clk_hw_register);
+EXPORT_SYMBOL_GPL(clk_register);
 
 /*
  * of_clk_hw_register - register a clk_hw and return an error code
@@ -3892,7 +3903,7 @@  EXPORT_SYMBOL_GPL(clk_hw_register);
  */
 int of_clk_hw_register(struct device_node *node, struct clk_hw *hw)
 {
-	return PTR_ERR_OR_ZERO(__clk_register(NULL, node, hw));
+	return __clk_hw_register(NULL, node, hw);
 }
 EXPORT_SYMBOL_GPL(of_clk_hw_register);
 
@@ -3972,25 +3983,25 @@  static void clk_core_evict_parent_cache(struct clk_core *core)
 }
 
 /**
- * clk_unregister - unregister a currently registered clock
- * @clk: clock to unregister
+ * clk_hw_unregister - unregister a currently registered clk_hw
+ * @hw: hardware-specific clock data to unregister
  */
-void clk_unregister(struct clk *clk)
+void clk_hw_unregister(struct clk_hw *hw)
 {
 	unsigned long flags;
 	const struct clk_ops *ops;
 
-	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+	if (!hw || WARN_ON_ONCE(IS_ERR(hw)))
 		return;
 
-	clk_debug_unregister(clk->core);
+	clk_debug_unregister(hw->core);
 
 	clk_prepare_lock();
 
-	ops = clk->core->ops;
+	ops = hw->core->ops;
 	if (ops == &clk_nodrv_ops) {
 		pr_err("%s: unregistered clock: %s\n", __func__,
-		       clk->core->name);
+		       hw->core->name);
 		goto unlock;
 	}
 	/*
@@ -3998,50 +4009,53 @@  void clk_unregister(struct clk *clk)
 	 * a reference to this clock.
 	 */
 	flags = clk_enable_lock();
-	clk->core->ops = &clk_nodrv_ops;
+	hw->core->ops = &clk_nodrv_ops;
 	clk_enable_unlock(flags);
 
 	if (ops->terminate)
-		ops->terminate(clk->core->hw);
+		ops->terminate(hw);
 
-	if (!hlist_empty(&clk->core->children)) {
+	if (!hlist_empty(&hw->core->children)) {
 		struct clk_core *child;
 		struct hlist_node *t;
 
 		/* Reparent all children to the orphan list. */
-		hlist_for_each_entry_safe(child, t, &clk->core->children,
+		hlist_for_each_entry_safe(child, t, &hw->core->children,
 					  child_node)
 			clk_core_set_parent_nolock(child, NULL);
 	}
 
-	clk_core_evict_parent_cache(clk->core);
+	clk_core_evict_parent_cache(hw->core);
 
-	hlist_del_init(&clk->core->child_node);
+	hlist_del_init(&hw->core->child_node);
 
-	if (clk->core->prepare_count)
+	if (hw->core->prepare_count)
 		pr_warn("%s: unregistering prepared clock: %s\n",
-					__func__, clk->core->name);
+					__func__, hw->core->name);
 
-	if (clk->core->protect_count)
+	if (hw->core->protect_count)
 		pr_warn("%s: unregistering protected clock: %s\n",
-					__func__, clk->core->name);
+					__func__, hw->core->name);
 
-	kref_put(&clk->core->ref, __clk_release);
-	free_clk(clk);
+	kref_put(&hw->core->ref, __clk_release);
+	free_clk(hw->clk);
 unlock:
 	clk_prepare_unlock();
 }
-EXPORT_SYMBOL_GPL(clk_unregister);
+EXPORT_SYMBOL_GPL(clk_hw_unregister);
 
 /**
- * clk_hw_unregister - unregister a currently registered clk_hw
- * @hw: hardware-specific clock data to unregister
+ * clk_unregister - unregister a currently registered clock
+ * @clk: clock to unregister
  */
-void clk_hw_unregister(struct clk_hw *hw)
+void clk_unregister(struct clk *clk)
 {
-	clk_unregister(hw->clk);
+	struct clk_hw *hw = clk->core->hw;
+
+	clk_put(clk);
+	clk_hw_unregister(hw);
 }
-EXPORT_SYMBOL_GPL(clk_hw_unregister);
+EXPORT_SYMBOL_GPL(clk_unregister);
 
 static void devm_clk_release(struct device *dev, void *res)
 {