* [PATCH v2 0/8] Rewrite clk parent handling
@ 2019-02-26 22:34 Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 1/8] clk: Combine __clk_get() and __clk_create_clk() Stephen Boyd
` (8 more replies)
0 siblings, 9 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
There are a couple warts with clk parent handling in the common clk
framework. This patch series addresses one of those warts, parent-child
linkages. We use strings for all parent-child linkages, and this leads
to poorly written code that extracts clk names from struct clk pointers
and makes clk provider drivers use clk consumer APIs.
Also, clkdev.c has a collection of DT parsing logic in it that is only
used when the common clk framework is present but we want to use that
same logic for describing parent-child linkages of clk providers via DT.
This code should all be moved into the common clk framework and used
from there as well as from clkdev.c, so this series changes the way
clkdev interacts with the clk framework by having clkdev get struct
clk_hw pointers out of DT clk specifiers and then convert those into clk
pointers with clk_hw_create_clk(). Splitting the API this way lets us
get struct clk_hw pointers for clk providers and skip the struct clk
pointer creation phase that we don't need to do when describing
parent-child linkages.
And finally, we have a few patches in here that lay the groundwork for
supporting device links in the common clk framework. We do that by
pushing the consuming device pointer through to the clk pointer creation
in clk_hw_create_clk(). This wasn't always easy to do when we had
__clk_create_clk() called from multiple places, some being deep in the
clk registration path. This series simplifies that logic so that we can
always attach a consumer device to a clk that we create in one place,
instead of making that linkage in multiple places near where we create
struct clk pointers.
I plan to at least merge the early patches in this series into clk-next
so we can clear the way for the later patches. I don't think anything is
too controversial until we get to the new way of specifying parents
at the end and of course, the sdm845 patch is incomplete so it's not
going to be merged.
Changes from v1:
* Dropped new get_parent_hw, we'll pick it up in a later series
* Rebased to v5.0-rc6 to avoid conflicts with clk-fixes
* Renamed 'fallback' to 'name' and 'name' to 'fw_name' in parent map
* Made sure that clk_hw_get_parent_by_index() never sees an error pointer
so that we don't mistakenly try to defer an error pointer
* Fixed index passing mistake on of_clk_get_hw_from_clkspec()
* Copy over all the data from parent maps and free it correctly too
Miquel Raynal (1):
clk: core: clarify the check for runtime PM
Stephen Boyd (7):
clk: Combine __clk_get() and __clk_create_clk()
clk: Introduce of_clk_get_hw_from_clkspec()
clk: Inform the core about consumer devices
clk: Move of_clk_*() APIs into clk.c from clkdev.c
clk: Allow parents to be specified without string names
clk: qcom: gcc-sdm845: Migrate to DT parent mapping
arm64: dts: qcom: Specify XO clk as input to GCC node
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +
drivers/clk/clk.c | 512 +++++++++++++++++++++------
drivers/clk/clk.h | 23 +-
drivers/clk/clkdev.c | 120 +------
drivers/clk/qcom/gcc-sdm845.c | 180 +++++-----
include/linux/clk-provider.h | 19 +
6 files changed, 532 insertions(+), 324 deletions(-)
base-commit: d13937116f1e82bf508a6325111b322c30c85eb9
--
Sent by a computer through tubes
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/8] clk: Combine __clk_get() and __clk_create_clk()
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
@ 2019-02-26 22:34 ` Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 2/8] clk: core: clarify the check for runtime PM Stephen Boyd
` (7 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
The __clk_get() function is practically a private clk implementation
detail now. No architecture defines it, and given that new code should
be using the common clk framework there isn't a need for it to keep
existing just to serve clkdev purposes. Let's fold it into the
__clk_create_clk() function and make that a little more generic by
renaming it to clk_hw_create_clk(). This will allow the framework to
create a struct clk handle to a particular clk_hw pointer and link it up
as a consumer wherever that's needed.
Doing this also lets us get rid of the __clk_free_clk() API that had to
be kept in sync with __clk_put(). Splitting that API up into the "link
and unlink from consumer list" phase and "free the clk pointer" phase
allows us to reuse that logic in a couple places, simplifying the code.
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
drivers/clk/clk.c | 140 +++++++++++++++++++++++++++++--------------
drivers/clk/clk.h | 10 +---
drivers/clk/clkdev.c | 9 +--
3 files changed, 98 insertions(+), 61 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d2477a5058ac..fef937ea44f4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3209,42 +3209,103 @@ static int __clk_core_init(struct clk_core *core)
return ret;
}
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
+/**
+ * clk_core_link_consumer - Add a clk consumer to the list of consumers in a clk_core
+ * @core: clk to add consumer to
+ * @clk: consumer to link to a clk
+ */
+static void clk_core_link_consumer(struct clk_core *core, struct clk *clk)
+{
+ clk_prepare_lock();
+ hlist_add_head(&clk->clks_node, &core->clks);
+ clk_prepare_unlock();
+}
+
+/**
+ * clk_core_unlink_consumer - Remove a clk consumer from the list of consumers in a clk_core
+ * @clk: consumer to unlink
+ */
+static void clk_core_unlink_consumer(struct clk *clk)
+{
+ lockdep_assert_held(&prepare_lock);
+ hlist_del(&clk->clks_node);
+}
+
+/**
+ * alloc_clk - Allocate a clk consumer, but leave it unlinked to the clk_core
+ * @core: clk to allocate a consumer for
+ * @dev_id: string describing device name
+ * @con_id: connection ID string on device
+ *
+ * Returns: clk consumer left unlinked from the consumer list
+ */
+static struct clk *alloc_clk(struct clk_core *core, const char *dev_id,
const char *con_id)
{
struct clk *clk;
- /* This is to allow this function to be chained to others */
- if (IS_ERR_OR_NULL(hw))
- return ERR_CAST(hw);
-
clk = kzalloc(sizeof(*clk), GFP_KERNEL);
if (!clk)
return ERR_PTR(-ENOMEM);
- clk->core = hw->core;
+ clk->core = core;
clk->dev_id = dev_id;
clk->con_id = kstrdup_const(con_id, GFP_KERNEL);
clk->max_rate = ULONG_MAX;
- clk_prepare_lock();
- hlist_add_head(&clk->clks_node, &hw->core->clks);
- clk_prepare_unlock();
-
return clk;
}
-/* keep in sync with __clk_put */
-void __clk_free_clk(struct clk *clk)
+/**
+ * free_clk - Free a clk consumer
+ * @clk: clk consumer to free
+ *
+ * Note, this assumes the clk has been unlinked from the clk_core consumer
+ * list.
+ */
+static void free_clk(struct clk *clk)
{
- clk_prepare_lock();
- hlist_del(&clk->clks_node);
- clk_prepare_unlock();
-
kfree_const(clk->con_id);
kfree(clk);
}
+/**
+ * clk_hw_create_clk: Allocate and link a clk consumer to a clk_core given
+ * a clk_hw
+ * @hw: clk_hw associated with the clk being consumed
+ * @dev_id: string describing device name
+ * @con_id: connection ID string on device
+ *
+ * This is the main function used to create a clk pointer for use by clk
+ * consumers. It connects a consumer to the clk_core and clk_hw structures
+ * used by the framework and clk provider respectively.
+ */
+struct clk *clk_hw_create_clk(struct clk_hw *hw,
+ const char *dev_id, const char *con_id)
+{
+ struct clk *clk;
+ struct clk_core *core;
+
+ /* This is to allow this function to be chained to others */
+ if (IS_ERR_OR_NULL(hw))
+ return ERR_CAST(hw);
+
+ core = hw->core;
+ clk = alloc_clk(core, dev_id, con_id);
+ if (IS_ERR(clk))
+ return clk;
+
+ if (!try_module_get(core->owner)) {
+ free_clk(clk);
+ return ERR_PTR(-ENOENT);
+ }
+
+ kref_get(&core->ref);
+ clk_core_link_consumer(core, clk);
+
+ return clk;
+}
+
/**
* clk_register - allocate a new clock, register it and return an opaque cookie
* @dev: device that is registering this clock
@@ -3320,17 +3381,27 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
INIT_HLIST_HEAD(&core->clks);
- hw->clk = __clk_create_clk(hw, NULL, NULL);
+ /*
+ * Don't call clk_hw_create_clk() here because that would pin the
+ * provider module to itself and prevent it from ever being removed.
+ */
+ hw->clk = alloc_clk(core, NULL, NULL);
if (IS_ERR(hw->clk)) {
ret = PTR_ERR(hw->clk);
goto fail_parents;
}
+ clk_core_link_consumer(hw->core, hw->clk);
+
ret = __clk_core_init(core);
if (!ret)
return hw->clk;
- __clk_free_clk(hw->clk);
+ clk_prepare_lock();
+ clk_core_unlink_consumer(hw->clk);
+ clk_prepare_unlock();
+
+ free_clk(hw->clk);
hw->clk = NULL;
fail_parents:
@@ -3601,20 +3672,7 @@ EXPORT_SYMBOL_GPL(devm_clk_hw_unregister);
/*
* clkdev helpers
*/
-int __clk_get(struct clk *clk)
-{
- struct clk_core *core = !clk ? NULL : clk->core;
-
- if (core) {
- if (!try_module_get(core->owner))
- return 0;
-
- kref_get(&core->ref);
- }
- return 1;
-}
-/* keep in sync with __clk_free_clk */
void __clk_put(struct clk *clk)
{
struct module *owner;
@@ -3648,8 +3706,7 @@ void __clk_put(struct clk *clk)
module_put(owner);
- kfree_const(clk->con_id);
- kfree(clk);
+ free_clk(clk);
}
/*** clk rate change notifiers ***/
@@ -4025,8 +4082,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
const char *dev_id, const char *con_id)
{
struct of_clk_provider *provider;
- struct clk *clk = ERR_PTR(-EPROBE_DEFER);
- struct clk_hw *hw;
+ struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
if (!clkspec)
return ERR_PTR(-EINVAL);
@@ -4036,21 +4092,13 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
list_for_each_entry(provider, &of_clk_providers, link) {
if (provider->node == clkspec->np) {
hw = __of_clk_get_hw_from_provider(provider, clkspec);
- clk = __clk_create_clk(hw, dev_id, con_id);
- }
-
- if (!IS_ERR(clk)) {
- if (!__clk_get(clk)) {
- __clk_free_clk(clk);
- clk = ERR_PTR(-ENOENT);
- }
-
- break;
+ if (!IS_ERR(hw))
+ break;
}
}
mutex_unlock(&of_clk_mutex);
- return clk;
+ return clk_hw_create_clk(hw, dev_id, con_id);
}
/**
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index b02f5e604e69..4cdf30b0008c 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -12,24 +12,20 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
#endif
#ifdef CONFIG_COMMON_CLK
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
- const char *con_id);
-void __clk_free_clk(struct clk *clk);
-int __clk_get(struct clk *clk);
+struct clk *clk_hw_create_clk(struct clk_hw *hw,
+ const char *dev_id, const char *con_id);
void __clk_put(struct clk *clk);
#else
/* All these casts to avoid ifdefs in clkdev... */
static inline struct clk *
-__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
+clk_hw_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
{
return (struct clk *)hw;
}
-static inline void __clk_free_clk(struct clk *clk) { }
static struct clk_hw *__clk_get_hw(struct clk *clk)
{
return (struct clk_hw *)clk;
}
-static inline int __clk_get(struct clk *clk) { return 1; }
static inline void __clk_put(struct clk *clk) { }
#endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8b3988..bdeaffc950ae 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -174,16 +174,9 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
if (!cl)
goto out;
- clk = __clk_create_clk(cl->clk_hw, dev_id, con_id);
+ clk = clk_hw_create_clk(cl->clk_hw, dev_id, con_id);
if (IS_ERR(clk))
- goto out;
-
- if (!__clk_get(clk)) {
- __clk_free_clk(clk);
cl = NULL;
- goto out;
- }
-
out:
mutex_unlock(&clocks_mutex);
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/8] clk: core: clarify the check for runtime PM
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 1/8] clk: Combine __clk_get() and __clk_create_clk() Stephen Boyd
@ 2019-02-26 22:34 ` Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 3/8] clk: Introduce of_clk_get_hw_from_clkspec() Stephen Boyd
` (6 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Miquel Raynal, linux-kernel, linux-clk, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
From: Miquel Raynal <miquel.raynal@bootlin.com>
Currently, the core->dev entry is populated only if runtime PM is
enabled. Doing so prevents accessing the device structure in any
case.
Keep the same logic but instead of using the presence of core->dev as
the only condition, also check the status of
pm_runtime_enabled(). Then, we can set the core->dev pointer at any
time as long as a device structure is available.
This change will help supporting device links in the clock subsystem.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
[sboyd@kernel.org: Change to a boolean flag]
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
drivers/clk/clk.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fef937ea44f4..d46e8b9b9c9f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -57,6 +57,7 @@ struct clk_core {
struct clk_core *new_child;
unsigned long flags;
bool orphan;
+ bool rpm_enabled;
unsigned int enable_count;
unsigned int prepare_count;
unsigned int protect_count;
@@ -92,9 +93,9 @@ struct clk {
/*** runtime pm ***/
static int clk_pm_runtime_get(struct clk_core *core)
{
- int ret = 0;
+ int ret;
- if (!core->dev)
+ if (!core->rpm_enabled)
return 0;
ret = pm_runtime_get_sync(core->dev);
@@ -103,7 +104,7 @@ static int clk_pm_runtime_get(struct clk_core *core)
static void clk_pm_runtime_put(struct clk_core *core)
{
- if (!core->dev)
+ if (!core->rpm_enabled)
return;
pm_runtime_put_sync(core->dev);
@@ -223,7 +224,7 @@ static bool clk_core_is_enabled(struct clk_core *core)
* taking enable spinlock, but the below check is needed if one tries
* to call it from other places.
*/
- if (core->dev) {
+ if (core->rpm_enabled) {
pm_runtime_get_noresume(core->dev);
if (!pm_runtime_active(core->dev)) {
ret = false;
@@ -233,7 +234,7 @@ static bool clk_core_is_enabled(struct clk_core *core)
ret = core->ops->is_enabled(core->hw);
done:
- if (core->dev)
+ if (core->rpm_enabled)
pm_runtime_put(core->dev);
return ret;
@@ -3341,7 +3342,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
core->ops = hw->init->ops;
if (dev && pm_runtime_enabled(dev))
- core->dev = dev;
+ core->rpm_enabled = true;
+ core->dev = dev;
if (dev && dev->driver)
core->owner = dev->driver->owner;
core->hw = hw;
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/8] clk: Introduce of_clk_get_hw_from_clkspec()
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 1/8] clk: Combine __clk_get() and __clk_create_clk() Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 2/8] clk: core: clarify the check for runtime PM Stephen Boyd
@ 2019-02-26 22:34 ` Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 4/8] clk: Inform the core about consumer devices Stephen Boyd
` (5 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
We want to get struct clk_hw pointers from a DT clk specifier (i.e. a
clocks property) so that we can find parent clks without searching for
globally unique clk names. This should save time by avoiding the global
string search for clks that are external to the clock controller
providing the clk and let us move away from string comparisons in
general.
Introduce of_clk_get_hw_from_clkspec() which is largely the DT parsing
part of finding clks implemented in clkdev.c and have that return a
clk_hw pointer instead of converting that into a clk pointer. This lets
us push up the clk pointer creation to the caller in clk_get() and
avoids the need to push the dev_id and con_id throughout the DT parsing
code.
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
drivers/clk/clk.c | 46 +++++++++++++++++++++---
drivers/clk/clk.h | 5 +--
drivers/clk/clkdev.c | 83 +++++++++++++-------------------------------
3 files changed, 69 insertions(+), 65 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d46e8b9b9c9f..04288063847b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4065,6 +4065,42 @@ void devm_of_clk_del_provider(struct device *dev)
}
EXPORT_SYMBOL(devm_of_clk_del_provider);
+int of_parse_clkspec(const struct device_node *np, int index, const char *name,
+ struct of_phandle_args *out_args)
+{
+ int ret = -ENOENT;
+
+ /* Walk up the tree of devices looking for a clock property that matches */
+ while (np) {
+ /*
+ * For named clocks, first look up the name in the
+ * "clock-names" property. If it cannot be found, then index
+ * will be an error code and of_parse_phandle_with_args() will
+ * return -EINVAL.
+ */
+ if (name)
+ index = of_property_match_string(np, "clock-names", name);
+ ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
+ index, out_args);
+ if (!ret)
+ break;
+ if (name && index >= 0)
+ break;
+
+ /*
+ * No matching clock found on this node. If the parent node
+ * has a "clock-ranges" property, then we can try one of its
+ * clocks.
+ */
+ np = np->parent;
+ if (np && !of_get_property(np, "clock-ranges", NULL))
+ break;
+ index = 0;
+ }
+
+ return ret;
+}
+
static struct clk_hw *
__of_clk_get_hw_from_provider(struct of_clk_provider *provider,
struct of_phandle_args *clkspec)
@@ -4080,8 +4116,7 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
return __clk_get_hw(clk);
}
-struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
- const char *dev_id, const char *con_id)
+struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
{
struct of_clk_provider *provider;
struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
@@ -4089,7 +4124,6 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
if (!clkspec)
return ERR_PTR(-EINVAL);
- /* Check if we have such a provider in our array */
mutex_lock(&of_clk_mutex);
list_for_each_entry(provider, &of_clk_providers, link) {
if (provider->node == clkspec->np) {
@@ -4100,7 +4134,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
}
mutex_unlock(&of_clk_mutex);
- return clk_hw_create_clk(hw, dev_id, con_id);
+ return hw;
}
/**
@@ -4113,7 +4147,9 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
*/
struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
{
- return __of_clk_get_from_provider(clkspec, NULL, __func__);
+ struct clk_hw *hw = of_clk_get_hw_from_clkspec(clkspec);
+
+ return clk_hw_create_clk(hw, NULL, __func__);
}
EXPORT_SYMBOL_GPL(of_clk_get_from_provider);
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 4cdf30b0008c..5a0ca0e3c1f1 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -7,8 +7,9 @@
struct clk_hw;
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
- const char *dev_id, const char *con_id);
+int of_parse_clkspec(const struct device_node *np, int index, const char *name,
+ struct of_phandle_args *out_args);
+struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec);
#endif
#ifdef CONFIG_COMMON_CLK
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index bdeaffc950ae..5ebb2119c0b9 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -28,69 +28,37 @@ static LIST_HEAD(clocks);
static DEFINE_MUTEX(clocks_mutex);
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-static struct clk *__of_clk_get(struct device_node *np, int index,
- const char *dev_id, const char *con_id)
+static struct clk_hw *of_clk_get_hw(struct device_node *np,
+ int index, const char *con_id)
{
+ int ret;
+ struct clk_hw *hw;
struct of_phandle_args clkspec;
- struct clk *clk;
- int rc;
- rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
- &clkspec);
- if (rc)
- return ERR_PTR(rc);
+ ret = of_parse_clkspec(np, index, con_id, &clkspec);
+ if (ret)
+ return ERR_PTR(ret);
- clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id);
+ hw = of_clk_get_hw_from_clkspec(&clkspec);
of_node_put(clkspec.np);
- return clk;
+ return hw;
}
-struct clk *of_clk_get(struct device_node *np, int index)
+static struct clk *__of_clk_get(struct device_node *np,
+ int index, const char *dev_id,
+ const char *con_id)
{
- return __of_clk_get(np, index, np->full_name, NULL);
+ struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
+
+ return clk_hw_create_clk(hw, dev_id, con_id);
}
-EXPORT_SYMBOL(of_clk_get);
-static struct clk *__of_clk_get_by_name(struct device_node *np,
- const char *dev_id,
- const char *name)
+struct clk *of_clk_get(struct device_node *np, int index)
{
- struct clk *clk = ERR_PTR(-ENOENT);
-
- /* Walk up the tree of devices looking for a clock that matches */
- while (np) {
- int index = 0;
-
- /*
- * For named clocks, first look up the name in the
- * "clock-names" property. If it cannot be found, then
- * index will be an error code, and of_clk_get() will fail.
- */
- if (name)
- index = of_property_match_string(np, "clock-names", name);
- clk = __of_clk_get(np, index, dev_id, name);
- if (!IS_ERR(clk)) {
- break;
- } else if (name && index >= 0) {
- if (PTR_ERR(clk) != -EPROBE_DEFER)
- pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
- np, name ? name : "", index);
- return clk;
- }
-
- /*
- * No matching clock found on this node. If the parent node
- * has a "clock-ranges" property, then we can try one of its
- * clocks.
- */
- np = np->parent;
- if (np && !of_get_property(np, "clock-ranges", NULL))
- break;
- }
-
- return clk;
+ return __of_clk_get(np, index, np->full_name, NULL);
}
+EXPORT_SYMBOL(of_clk_get);
/**
* of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
@@ -106,15 +74,14 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
if (!np)
return ERR_PTR(-ENOENT);
- return __of_clk_get_by_name(np, np->full_name, name);
+ return __of_clk_get(np, -1, np->full_name, name);
}
EXPORT_SYMBOL(of_clk_get_by_name);
#else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
-static struct clk *__of_clk_get_by_name(struct device_node *np,
- const char *dev_id,
- const char *name)
+static struct clk_hw *of_clk_get_hw(struct device_node *np,
+ int index, const char *con_id)
{
return ERR_PTR(-ENOENT);
}
@@ -187,12 +154,12 @@ EXPORT_SYMBOL(clk_get_sys);
struct clk *clk_get(struct device *dev, const char *con_id)
{
const char *dev_id = dev ? dev_name(dev) : NULL;
- struct clk *clk;
+ struct clk_hw *hw;
if (dev && dev->of_node) {
- clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
- if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
- return clk;
+ hw = of_clk_get_hw(dev->of_node, 0, con_id);
+ if (!IS_ERR(hw) || PTR_ERR(hw) == -EPROBE_DEFER)
+ return clk_hw_create_clk(hw, dev_id, con_id);
}
return clk_get_sys(dev_id, con_id);
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 4/8] clk: Inform the core about consumer devices
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
` (2 preceding siblings ...)
2019-02-26 22:34 ` [PATCH v2 3/8] clk: Introduce of_clk_get_hw_from_clkspec() Stephen Boyd
@ 2019-02-26 22:34 ` Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 5/8] clk: Move of_clk_*() APIs into clk.c from clkdev.c Stephen Boyd
` (4 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
We'd like to have a pointer to the device that's consuming a particular
clk in the clk framework so we can link the consumer to the clk provider
with a PM device link. Add a device argument to clk_hw_create_clk() for
this so it can be used in subsequent patches to add and remove the link.
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
drivers/clk/clk.c | 7 +++++--
drivers/clk/clk.h | 7 +++++--
drivers/clk/clkdev.c | 16 +++++++++++-----
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 04288063847b..8244ef2ba977 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -82,6 +82,7 @@ struct clk_core {
struct clk {
struct clk_core *core;
+ struct device *dev;
const char *dev_id;
const char *con_id;
unsigned long min_rate;
@@ -3273,6 +3274,7 @@ static void free_clk(struct clk *clk)
/**
* clk_hw_create_clk: Allocate and link a clk consumer to a clk_core given
* a clk_hw
+ * @dev: clk consumer device
* @hw: clk_hw associated with the clk being consumed
* @dev_id: string describing device name
* @con_id: connection ID string on device
@@ -3281,7 +3283,7 @@ static void free_clk(struct clk *clk)
* consumers. It connects a consumer to the clk_core and clk_hw structures
* used by the framework and clk provider respectively.
*/
-struct clk *clk_hw_create_clk(struct clk_hw *hw,
+struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
const char *dev_id, const char *con_id)
{
struct clk *clk;
@@ -3295,6 +3297,7 @@ struct clk *clk_hw_create_clk(struct clk_hw *hw,
clk = alloc_clk(core, dev_id, con_id);
if (IS_ERR(clk))
return clk;
+ clk->dev = dev;
if (!try_module_get(core->owner)) {
free_clk(clk);
@@ -4149,7 +4152,7 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
{
struct clk_hw *hw = of_clk_get_hw_from_clkspec(clkspec);
- return clk_hw_create_clk(hw, NULL, __func__);
+ return clk_hw_create_clk(NULL, hw, NULL, __func__);
}
EXPORT_SYMBOL_GPL(of_clk_get_from_provider);
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 5a0ca0e3c1f1..5ea2185e57a1 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -5,6 +5,8 @@
*/
struct clk_hw;
+struct device;
+struct of_phandle_args;
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
int of_parse_clkspec(const struct device_node *np, int index, const char *name,
@@ -13,13 +15,14 @@ struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec);
#endif
#ifdef CONFIG_COMMON_CLK
-struct clk *clk_hw_create_clk(struct clk_hw *hw,
+struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
const char *dev_id, const char *con_id);
void __clk_put(struct clk *clk);
#else
/* All these casts to avoid ifdefs in clkdev... */
static inline struct clk *
-clk_hw_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
+clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id,
+ const char *con_id)
{
return (struct clk *)hw;
}
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 5ebb2119c0b9..f2f4f2afd28c 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -51,7 +51,7 @@ static struct clk *__of_clk_get(struct device_node *np,
{
struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
- return clk_hw_create_clk(hw, dev_id, con_id);
+ return clk_hw_create_clk(NULL, hw, dev_id, con_id);
}
struct clk *of_clk_get(struct device_node *np, int index)
@@ -130,7 +130,8 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
return cl;
}
-struct clk *clk_get_sys(const char *dev_id, const char *con_id)
+static struct clk *__clk_get_sys(struct device *dev, const char *dev_id,
+ const char *con_id)
{
struct clk_lookup *cl;
struct clk *clk = NULL;
@@ -141,7 +142,7 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
if (!cl)
goto out;
- clk = clk_hw_create_clk(cl->clk_hw, dev_id, con_id);
+ clk = clk_hw_create_clk(dev, cl->clk_hw, dev_id, con_id);
if (IS_ERR(clk))
cl = NULL;
out:
@@ -149,6 +150,11 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
return cl ? clk : ERR_PTR(-ENOENT);
}
+
+struct clk *clk_get_sys(const char *dev_id, const char *con_id)
+{
+ return __clk_get_sys(NULL, dev_id, con_id);
+}
EXPORT_SYMBOL(clk_get_sys);
struct clk *clk_get(struct device *dev, const char *con_id)
@@ -159,10 +165,10 @@ struct clk *clk_get(struct device *dev, const char *con_id)
if (dev && dev->of_node) {
hw = of_clk_get_hw(dev->of_node, 0, con_id);
if (!IS_ERR(hw) || PTR_ERR(hw) == -EPROBE_DEFER)
- return clk_hw_create_clk(hw, dev_id, con_id);
+ return clk_hw_create_clk(dev, hw, dev_id, con_id);
}
- return clk_get_sys(dev_id, con_id);
+ return __clk_get_sys(dev, dev_id, con_id);
}
EXPORT_SYMBOL(clk_get);
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 5/8] clk: Move of_clk_*() APIs into clk.c from clkdev.c
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
` (3 preceding siblings ...)
2019-02-26 22:34 ` [PATCH v2 4/8] clk: Inform the core about consumer devices Stephen Boyd
@ 2019-02-26 22:34 ` Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 6/8] clk: Allow parents to be specified without string names Stephen Boyd
` (3 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
The API between clk.c and clkdev.c is purely getting the clk_hw
structure (or the struct clk if it's not CCF) and then turning that
struct clk_hw pointer into a struct clk pointer via clk_hw_create_clk().
There's no need to complicate clkdev.c with these DT parsing details
that are only relevant to the common clk framework. Move the DT parsing
logic into the core framework and just expose the APIs to get a clk_hw
pointer and convert it.
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
drivers/clk/clk.c | 57 ++++++++++++++++++++++++++++++++++++++---
drivers/clk/clk.h | 11 +++++---
drivers/clk/clkdev.c | 60 --------------------------------------------
3 files changed, 62 insertions(+), 66 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8244ef2ba977..937b8d092d17 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4068,8 +4068,8 @@ void devm_of_clk_del_provider(struct device *dev)
}
EXPORT_SYMBOL(devm_of_clk_del_provider);
-int of_parse_clkspec(const struct device_node *np, int index, const char *name,
- struct of_phandle_args *out_args)
+static int of_parse_clkspec(const struct device_node *np, int index,
+ const char *name, struct of_phandle_args *out_args)
{
int ret = -ENOENT;
@@ -4119,7 +4119,8 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
return __clk_get_hw(clk);
}
-struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
+static struct clk_hw *
+of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
{
struct of_clk_provider *provider;
struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
@@ -4156,6 +4157,56 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
}
EXPORT_SYMBOL_GPL(of_clk_get_from_provider);
+struct clk_hw *of_clk_get_hw(struct device_node *np, int index,
+ const char *con_id)
+{
+ int ret;
+ struct clk_hw *hw;
+ struct of_phandle_args clkspec;
+
+ ret = of_parse_clkspec(np, index, con_id, &clkspec);
+ if (ret)
+ return ERR_PTR(ret);
+
+ hw = of_clk_get_hw_from_clkspec(&clkspec);
+ of_node_put(clkspec.np);
+
+ return hw;
+}
+
+static struct clk *__of_clk_get(struct device_node *np,
+ int index, const char *dev_id,
+ const char *con_id)
+{
+ struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
+
+ return clk_hw_create_clk(NULL, hw, dev_id, con_id);
+}
+
+struct clk *of_clk_get(struct device_node *np, int index)
+{
+ return __of_clk_get(np, index, np->full_name, NULL);
+}
+EXPORT_SYMBOL(of_clk_get);
+
+/**
+ * of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
+ * @np: pointer to clock consumer node
+ * @name: name of consumer's clock input, or NULL for the first clock reference
+ *
+ * This function parses the clocks and clock-names properties,
+ * and uses them to look up the struct clk from the registered list of clock
+ * providers.
+ */
+struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
+{
+ if (!np)
+ return ERR_PTR(-ENOENT);
+
+ return __of_clk_get(np, -1, np->full_name, name);
+}
+EXPORT_SYMBOL(of_clk_get_by_name);
+
/**
* of_clk_get_parent_count() - Count the number of clocks a device node has
* @np: device node to count
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 5ea2185e57a1..553f531cc232 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -9,9 +9,14 @@ struct device;
struct of_phandle_args;
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-int of_parse_clkspec(const struct device_node *np, int index, const char *name,
- struct of_phandle_args *out_args);
-struct clk_hw *of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec);
+struct clk_hw *of_clk_get_hw(struct device_node *np,
+ int index, const char *con_id);
+#else /* !CONFIG_COMMON_CLK || !CONFIG_OF */
+static inline struct clk_hw *of_clk_get_hw(struct device_node *np,
+ int index, const char *con_id)
+{
+ return ERR_PTR(-ENOENT);
+}
#endif
#ifdef CONFIG_COMMON_CLK
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index f2f4f2afd28c..d3758bf4305c 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -27,66 +27,6 @@
static LIST_HEAD(clocks);
static DEFINE_MUTEX(clocks_mutex);
-#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-static struct clk_hw *of_clk_get_hw(struct device_node *np,
- int index, const char *con_id)
-{
- int ret;
- struct clk_hw *hw;
- struct of_phandle_args clkspec;
-
- ret = of_parse_clkspec(np, index, con_id, &clkspec);
- if (ret)
- return ERR_PTR(ret);
-
- hw = of_clk_get_hw_from_clkspec(&clkspec);
- of_node_put(clkspec.np);
-
- return hw;
-}
-
-static struct clk *__of_clk_get(struct device_node *np,
- int index, const char *dev_id,
- const char *con_id)
-{
- struct clk_hw *hw = of_clk_get_hw(np, index, con_id);
-
- return clk_hw_create_clk(NULL, hw, dev_id, con_id);
-}
-
-struct clk *of_clk_get(struct device_node *np, int index)
-{
- return __of_clk_get(np, index, np->full_name, NULL);
-}
-EXPORT_SYMBOL(of_clk_get);
-
-/**
- * of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
- * @np: pointer to clock consumer node
- * @name: name of consumer's clock input, or NULL for the first clock reference
- *
- * This function parses the clocks and clock-names properties,
- * and uses them to look up the struct clk from the registered list of clock
- * providers.
- */
-struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
-{
- if (!np)
- return ERR_PTR(-ENOENT);
-
- return __of_clk_get(np, -1, np->full_name, name);
-}
-EXPORT_SYMBOL(of_clk_get_by_name);
-
-#else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
-
-static struct clk_hw *of_clk_get_hw(struct device_node *np,
- int index, const char *con_id)
-{
- return ERR_PTR(-ENOENT);
-}
-#endif
-
/*
* Find the correct struct clk for the device and connection ID.
* We do slightly fuzzy matching here:
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
` (4 preceding siblings ...)
2019-02-26 22:34 ` [PATCH v2 5/8] clk: Move of_clk_*() APIs into clk.c from clkdev.c Stephen Boyd
@ 2019-02-26 22:34 ` Stephen Boyd
2019-03-02 18:40 ` Jerome Brunet
` (2 more replies)
2019-02-26 22:34 ` [PATCH v2 7/8] clk: qcom: gcc-sdm845: Migrate to DT parent mapping Stephen Boyd
` (2 subsequent siblings)
8 siblings, 3 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
The common clk framework is lacking in ability to describe the clk
topology without specifying strings for every possible parent-child
link. There are a few drawbacks to the current approach:
1) String comparisons are used for everything, including describing
topologies that are 'local' to a single clock controller.
2) clk providers (e.g. i2c clk drivers) need to create globally unique
clk names to avoid collisions in the clk namespace, leading to awkward
name generation code in various clk drivers.
3) DT bindings may not fully describe the clk topology and linkages
between clk controllers because drivers can easily rely on globally unique
strings to describe connections between clks.
This leads to confusing DT bindings, complicated clk name generation
code, and inefficient string comparisons during clk registration just so
that the clk framework can detect the topology of the clk tree.
Furthermore, some drivers call clk_get() and then __clk_get_name() to
extract the globally unique clk name just so they can specify the parent
of the clk they're registering. We have of_clk_parent_fill() but that
mostly only works for single clks registered from a DT node, which isn't
the norm. Let's simplify this all by introducing two new ways of
specifying clk parents.
The first method is an array of pointers to clk_hw structures
corresponding to the parents at that index. This works for clks that are
registered when we have access to all the clk_hw pointers for the
parents.
The second method is a mix of clk_hw pointers and strings of local and
global parent clk names. If the .name member of the map is set we'll
look for that clk by performing a DT based lookup of the device the clk
is registered with and the .name specified in the map. If that fails,
we'll fallback to the .fallback member and perform a global clk name
lookup like we've always done before.
Using either one of these new methods is entirely optional. Existing
drivers will continue to work, and they can migrate to this new approach
as they see fit. Eventually, we'll want to get rid of the 'parent_names'
array in struct clk_init_data and use one of these new methods instead.
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
drivers/clk/clk.c | 260 ++++++++++++++++++++++++++---------
include/linux/clk-provider.h | 19 +++
2 files changed, 217 insertions(+), 62 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 937b8d092d17..3d01e8c56400 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list);
/*** private data structures ***/
+struct clk_parent_map {
+ const struct clk_hw *hw;
+ struct clk_core *core;
+ const char *fw_name;
+ const char *name;
+};
+
struct clk_core {
const char *name;
const struct clk_ops *ops;
@@ -46,8 +53,7 @@ struct clk_core {
struct module *owner;
struct device *dev;
struct clk_core *parent;
- const char **parent_names;
- struct clk_core **parents;
+ struct clk_parent_map *parents;
u8 num_parents;
u8 new_parent_index;
unsigned long rate;
@@ -316,17 +322,92 @@ static struct clk_core *clk_core_lookup(const char *name)
return NULL;
}
+/**
+ * clk_core_get - Find the parent of a clk using a clock specifier in DT
+ * @core: clk to find parent of
+ * @name: name to search for in 'clock-names' of device providing clk
+ *
+ * This is the preferred method for clk providers to find the parent of a
+ * clk when that parent is external to the clk controller. The parent_names
+ * array is indexed and treated as a local name matching a string in the device
+ * node's 'clock-names' property. This allows clk providers to use their own
+ * namespace instead of looking for a globally unique parent string.
+ *
+ * For example the following DT snippet would allow a clock registered by the
+ * clock-controller@c001 that has a clk_init_data::parent_data array
+ * with 'xtal' in the 'name' member to find the clock provided by the
+ * clock-controller@f00abcd without needing to get the globally unique name of
+ * the xtal clk.
+ *
+ * parent: clock-controller@f00abcd {
+ * reg = <0xf00abcd 0xabcd>;
+ * #clock-cells = <0>;
+ * };
+ *
+ * clock-controller@c001 {
+ * reg = <0xc001 0xf00d>;
+ * clocks = <&parent>;
+ * clock-names = "xtal";
+ * #clock-cells = <1>;
+ * };
+ *
+ * Returns: -ENOENT when the provider can't be found or the clk doesn't
+ * exist in the provider. -EINVAL when the name can't be found. NULL when the
+ * provider knows about the clk but it isn't provided on this system.
+ * A valid clk_core pointer when the clk can be found in the provider.
+ */
+static struct clk_core *clk_core_get(struct clk_core *core, const char *name)
+{
+ struct clk_hw *hw;
+ struct device *dev = core->dev;
+
+ if (!dev)
+ return ERR_PTR(-ENOENT);
+
+ /* TODO: Support clkdev clk_lookups */
+ hw = of_clk_get_hw(dev->of_node, -1, name);
+ if (IS_ERR_OR_NULL(hw))
+ return ERR_CAST(hw);
+
+ return hw->core;
+}
+
+static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
+{
+ struct clk_parent_map *entry = &core->parents[index];
+ struct clk_core *parent = ERR_PTR(-ENOENT);
+
+ if (entry->hw) {
+ parent = entry->hw->core;
+ /*
+ * We have a direct reference but it isn't registered yet? Orphan it
+ * and let clk_reparent() update the orphan status when the parent
+ * is registered.
+ */
+ if (!parent)
+ parent = ERR_PTR(-EPROBE_DEFER);
+ } else {
+ if (entry->fw_name)
+ parent = clk_core_get(core, entry->fw_name);
+ if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
+ parent = clk_core_lookup(entry->name);
+ }
+
+ /* Only cache it if it's not an error */
+ if (!IS_ERR(parent))
+ entry->core = parent;
+}
+
static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
u8 index)
{
- if (!core || index >= core->num_parents)
+ if (!core || index >= core->num_parents || !core->parents)
return NULL;
- if (!core->parents[index])
- core->parents[index] =
- clk_core_lookup(core->parent_names[index]);
+ if (!core->parents[index].core)
+ clk_core_fill_parent_index(core, index);
- return core->parents[index];
+ return core->parents[index].core;
}
struct clk_hw *
@@ -1516,15 +1597,15 @@ static int clk_fetch_parent_index(struct clk_core *core,
return -EINVAL;
for (i = 0; i < core->num_parents; i++) {
- if (core->parents[i] == parent)
+ if (core->parents[i].core == parent)
return i;
- if (core->parents[i])
+ if (core->parents[i].core)
continue;
/* Fallback to comparing globally unique names */
- if (!strcmp(parent->name, core->parent_names[i])) {
- core->parents[i] = parent;
+ if (!strcmp(parent->name, core->parents[i].name)) {
+ core->parents[i].core = parent;
return i;
}
}
@@ -2290,6 +2371,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
bool clk_has_parent(struct clk *clk, struct clk *parent)
{
struct clk_core *core, *parent_core;
+ int i;
/* NULL clocks should be nops, so return success if either is NULL. */
if (!clk || !parent)
@@ -2302,8 +2384,11 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
if (core->parent == parent_core)
return true;
- return match_string(core->parent_names, core->num_parents,
- parent_core->name) >= 0;
+ for (i = 0; i < core->num_parents; i++)
+ if (!strcmp(core->parents[i].name, parent_core->name))
+ return true;
+
+ return false;
}
EXPORT_SYMBOL_GPL(clk_has_parent);
@@ -2886,9 +2971,9 @@ static int possible_parents_show(struct seq_file *s, void *data)
int i;
for (i = 0; i < core->num_parents - 1; i++)
- seq_printf(s, "%s ", core->parent_names[i]);
+ seq_printf(s, "%s ", core->parents[i].name);
- seq_printf(s, "%s\n", core->parent_names[i]);
+ seq_printf(s, "%s\n", core->parents[i].name);
return 0;
}
@@ -3022,7 +3107,7 @@ static inline void clk_debug_unregister(struct clk_core *core)
*/
static int __clk_core_init(struct clk_core *core)
{
- int i, ret;
+ int ret;
struct clk_core *orphan;
struct hlist_node *tmp2;
unsigned long rate;
@@ -3076,12 +3161,6 @@ static int __clk_core_init(struct clk_core *core)
goto out;
}
- /* throw a WARN if any entries in parent_names are NULL */
- for (i = 0; i < core->num_parents; i++)
- WARN(!core->parent_names[i],
- "%s: invalid NULL in %s's .parent_names\n",
- __func__, core->name);
-
core->parent = __clk_init_parent(core);
/*
@@ -3310,6 +3389,96 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
return clk;
}
+static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
+{
+ if (!src) {
+ if (must_exist)
+ return -EINVAL;
+ return 0;
+ }
+
+ dst = kstrdup_const(src, GFP_KERNEL);
+ if (!dst)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int clk_core_populate_parent_map(struct clk_core *core)
+{
+ const struct clk_init_data *init = core->hw->init;
+ u8 num_parents = init->num_parents;
+ const char * const *parent_names = init->parent_names;
+ const struct clk_hw **parent_hws = init->parent_hws;
+ const struct clk_parent_data *parent_data = init->parent_data;
+ int i, ret = 0;
+ struct clk_parent_map *parents, *parent;
+
+ if (!num_parents)
+ return 0;
+
+ /*
+ * Avoid unnecessary string look-ups of clk_core's possible parents by
+ * having a cache of names/clk_hw pointers to clk_core pointers.
+ */
+ parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
+ core->parents = parents;
+ if (!parents)
+ return -ENOMEM;
+
+ /* Copy everything over because it might be __initdata */
+ for (i = 0, parent = parents; i < num_parents; i++, parent++) {
+ if (parent_names) {
+ /* throw a WARN if any entries are NULL */
+ WARN(!parent_names[i],
+ "%s: invalid NULL in %s's .parent_names\n",
+ __func__, core->name);
+ ret = clk_cpy_name(parent->name, parent_names[i],
+ true);
+ } else if (parent_data) {
+ parent->hw = parent_data[i].hw;
+ ret = clk_cpy_name(parent->fw_name,
+ parent_data[i].fw_name, false);
+ if (!ret)
+ ret = clk_cpy_name(parent->name,
+ parent_data[i].name,
+ false);
+ } else if (parent_hws) {
+ parent->hw = parent_hws[i];
+ } else {
+ ret = -EINVAL;
+ WARN(1, "Must specify parents if num_parents > 0\n");
+ }
+
+ if (ret) {
+ do {
+ kfree_const(parents[i].name);
+ kfree_const(parents[i].fw_name);
+ } while (--i >= 0);
+ kfree(parents);
+
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void clk_core_free_parent_map(struct clk_core *core)
+{
+ int i = core->num_parents;
+
+ if (!core->num_parents)
+ return;
+
+ while (--i >= 0) {
+ kfree_const(core->parents[i].name);
+ kfree_const(core->parents[i].fw_name);
+ }
+
+ kfree(core->parents);
+}
+
/**
* clk_register - allocate a new clock, register it and return an opaque cookie
* @dev: device that is registering this clock
@@ -3323,7 +3492,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
*/
struct clk *clk_register(struct device *dev, struct clk_hw *hw)
{
- int i, ret;
+ int ret;
struct clk_core *core;
core = kzalloc(sizeof(*core), GFP_KERNEL);
@@ -3356,33 +3525,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
core->max_rate = ULONG_MAX;
hw->core = core;
- /* allocate local copy in case parent_names is __initdata */
- core->parent_names = kcalloc(core->num_parents, sizeof(char *),
- GFP_KERNEL);
-
- if (!core->parent_names) {
- ret = -ENOMEM;
- goto fail_parent_names;
- }
-
-
- /* copy each string name in case parent_names is __initdata */
- for (i = 0; i < core->num_parents; i++) {
- core->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
- GFP_KERNEL);
- if (!core->parent_names[i]) {
- ret = -ENOMEM;
- goto fail_parent_names_copy;
- }
- }
-
- /* avoid unnecessary string look-ups of clk_core's possible parents. */
- core->parents = kcalloc(core->num_parents, sizeof(*core->parents),
- GFP_KERNEL);
- if (!core->parents) {
- ret = -ENOMEM;
+ ret = clk_core_populate_parent_map(core);
+ if (ret)
goto fail_parents;
- };
INIT_HLIST_HEAD(&core->clks);
@@ -3393,7 +3538,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
hw->clk = alloc_clk(core, NULL, NULL);
if (IS_ERR(hw->clk)) {
ret = PTR_ERR(hw->clk);
- goto fail_parents;
+ goto fail_create_clk;
}
clk_core_link_consumer(hw->core, hw->clk);
@@ -3409,13 +3554,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
free_clk(hw->clk);
hw->clk = NULL;
+fail_create_clk:
+ clk_core_free_parent_map(core);
fail_parents:
- kfree(core->parents);
-fail_parent_names_copy:
- while (--i >= 0)
- kfree_const(core->parent_names[i]);
- kfree(core->parent_names);
-fail_parent_names:
fail_ops:
kfree_const(core->name);
fail_name:
@@ -3445,15 +3586,10 @@ EXPORT_SYMBOL_GPL(clk_hw_register);
static void __clk_release(struct kref *ref)
{
struct clk_core *core = container_of(ref, struct clk_core, ref);
- int i = core->num_parents;
lockdep_assert_held(&prepare_lock);
- kfree(core->parents);
- while (--i >= 0)
- kfree_const(core->parent_names[i]);
-
- kfree(core->parent_names);
+ clk_core_free_parent_map(core);
kfree_const(core->name);
kfree(core);
}
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index e443fa9fa859..f4de52c6764e 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -250,6 +250,18 @@ struct clk_ops {
void (*debug_init)(struct clk_hw *hw, struct dentry *dentry);
};
+/**
+ * struct clk_parent_data - clk parent information
+ * @hw: parent clk_hw pointer (used for clk providers with internal clks)
+ * @fw_name: parent name local to provider registering clk
+ * @name: globally unique parent name (used as a fallback)
+ */
+struct clk_parent_data {
+ const struct clk_hw *hw;
+ const char *fw_name;
+ const char *name;
+};
+
/**
* struct clk_init_data - holds init data that's common to all clocks and is
* shared between the clock provider and the common clock framework.
@@ -257,13 +269,20 @@ struct clk_ops {
* @name: clock name
* @ops: operations this clock supports
* @parent_names: array of string names for all possible parents
+ * @parent_data: array of parent data for all possible parents (when some
+ * parents are external to the clk controller)
+ * @parent_hws: array of pointers to all possible parents (when all parents
+ * are internal to the clk controller)
* @num_parents: number of possible parents
* @flags: framework-level hints and quirks
*/
struct clk_init_data {
const char *name;
const struct clk_ops *ops;
+ /* Only one of the following three should be assigned */
const char * const *parent_names;
+ const struct clk_parent_data *parent_data;
+ const struct clk_hw **parent_hws;
u8 num_parents;
unsigned long flags;
};
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 7/8] clk: qcom: gcc-sdm845: Migrate to DT parent mapping
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
` (5 preceding siblings ...)
2019-02-26 22:34 ` [PATCH v2 6/8] clk: Allow parents to be specified without string names Stephen Boyd
@ 2019-02-26 22:34 ` Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 8/8] arm64: dts: qcom: Specify XO clk as input to GCC node Stephen Boyd
2019-03-02 0:45 ` [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
8 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
TODO: Fully convert driver
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
drivers/clk/qcom/gcc-sdm845.c | 180 +++++++++++++++++-----------------
1 file changed, 90 insertions(+), 90 deletions(-)
diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
index 58fa5c247af1..90ce40e25f59 100644
--- a/drivers/clk/qcom/gcc-sdm845.c
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -25,6 +25,59 @@
#include "gdsc.h"
#include "reset.h"
+static struct clk_alpha_pll gpll0 = {
+ .offset = 0x0,
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+ .clkr = {
+ .enable_reg = 0x52000,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gpll0",
+ .parent_names = (const char *[]){ "bi_tcxo" },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_fixed_fabia_ops,
+ },
+ },
+};
+
+static struct clk_alpha_pll gpll4 = {
+ .offset = 0x76000,
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+ .clkr = {
+ .enable_reg = 0x52000,
+ .enable_mask = BIT(4),
+ .hw.init = &(struct clk_init_data){
+ .name = "gpll4",
+ .parent_names = (const char *[]){ "bi_tcxo" },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_fixed_fabia_ops,
+ },
+ },
+};
+
+static const struct clk_div_table post_div_table_fabia_even[] = {
+ { 0x0, 1 },
+ { 0x1, 2 },
+ { 0x3, 4 },
+ { 0x7, 8 },
+ { }
+};
+
+static struct clk_alpha_pll_postdiv gpll0_out_even = {
+ .offset = 0x0,
+ .post_div_shift = 8,
+ .post_div_table = post_div_table_fabia_even,
+ .num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
+ .width = 4,
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+ .clkr.hw.init = &(struct clk_init_data){
+ .name = "gpll0_out_even",
+ .parent_names = (const char *[]){ "gpll0" },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_postdiv_fabia_ops,
+ },
+};
+
enum {
P_BI_TCXO,
P_AUD_REF_CLK,
@@ -42,11 +95,11 @@ static const struct parent_map gcc_parent_map_0[] = {
{ P_CORE_BI_PLL_TEST_SE, 7 },
};
-static const char * const gcc_parent_names_0[] = {
- "bi_tcxo",
- "gpll0",
- "gpll0_out_even",
- "core_bi_pll_test_se",
+static const struct clk_parent_data gcc_parent_data_0[] = {
+ { .fw_name = "xo", .name = "bi_tcxo" },
+ { .hw = &gpll0.clkr.hw },
+ { .hw = &gpll0_out_even.clkr.hw },
+ { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }
};
static const struct parent_map gcc_parent_map_1[] = {
@@ -150,59 +203,6 @@ static const char * const gcc_parent_names_10[] = {
"core_bi_pll_test_se",
};
-static struct clk_alpha_pll gpll0 = {
- .offset = 0x0,
- .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
- .clkr = {
- .enable_reg = 0x52000,
- .enable_mask = BIT(0),
- .hw.init = &(struct clk_init_data){
- .name = "gpll0",
- .parent_names = (const char *[]){ "bi_tcxo" },
- .num_parents = 1,
- .ops = &clk_alpha_pll_fixed_fabia_ops,
- },
- },
-};
-
-static struct clk_alpha_pll gpll4 = {
- .offset = 0x76000,
- .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
- .clkr = {
- .enable_reg = 0x52000,
- .enable_mask = BIT(4),
- .hw.init = &(struct clk_init_data){
- .name = "gpll4",
- .parent_names = (const char *[]){ "bi_tcxo" },
- .num_parents = 1,
- .ops = &clk_alpha_pll_fixed_fabia_ops,
- },
- },
-};
-
-static const struct clk_div_table post_div_table_fabia_even[] = {
- { 0x0, 1 },
- { 0x1, 2 },
- { 0x3, 4 },
- { 0x7, 8 },
- { }
-};
-
-static struct clk_alpha_pll_postdiv gpll0_out_even = {
- .offset = 0x0,
- .post_div_shift = 8,
- .post_div_table = post_div_table_fabia_even,
- .num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
- .width = 4,
- .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
- .clkr.hw.init = &(struct clk_init_data){
- .name = "gpll0_out_even",
- .parent_names = (const char *[]){ "gpll0" },
- .num_parents = 1,
- .ops = &clk_alpha_pll_postdiv_fabia_ops,
- },
-};
-
static const struct freq_tbl ftbl_gcc_cpuss_ahb_clk_src[] = {
F(19200000, P_BI_TCXO, 1, 0, 0),
{ }
@@ -340,7 +340,7 @@ static struct clk_rcg2 gcc_pcie_phy_refgen_clk_src = {
.freq_tbl = ftbl_gcc_pcie_phy_refgen_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_pcie_phy_refgen_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -362,7 +362,7 @@ static struct clk_rcg2 gcc_qspi_core_clk_src = {
.freq_tbl = ftbl_gcc_qspi_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_qspi_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_floor_ops,
},
@@ -383,7 +383,7 @@ static struct clk_rcg2 gcc_pdm2_clk_src = {
.freq_tbl = ftbl_gcc_pdm2_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_pdm2_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -410,7 +410,7 @@ static const struct freq_tbl ftbl_gcc_qupv3_wrap0_s0_clk_src[] = {
static struct clk_init_data gcc_qupv3_wrap0_s0_clk_init = {
.name = "gcc_qupv3_wrap0_s0_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -426,7 +426,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s0_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s1_clk_init = {
.name = "gcc_qupv3_wrap0_s1_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -442,7 +442,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s1_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s2_clk_init = {
.name = "gcc_qupv3_wrap0_s2_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -458,7 +458,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s2_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s3_clk_init = {
.name = "gcc_qupv3_wrap0_s3_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -474,7 +474,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s3_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s4_clk_init = {
.name = "gcc_qupv3_wrap0_s4_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -490,7 +490,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s4_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s5_clk_init = {
.name = "gcc_qupv3_wrap0_s5_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -506,7 +506,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s5_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s6_clk_init = {
.name = "gcc_qupv3_wrap0_s6_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -522,7 +522,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s6_clk_src = {
static struct clk_init_data gcc_qupv3_wrap0_s7_clk_init = {
.name = "gcc_qupv3_wrap0_s7_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -538,7 +538,7 @@ static struct clk_rcg2 gcc_qupv3_wrap0_s7_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s0_clk_init = {
.name = "gcc_qupv3_wrap1_s0_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -554,7 +554,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s0_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s1_clk_init = {
.name = "gcc_qupv3_wrap1_s1_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -570,7 +570,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s1_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s2_clk_init = {
.name = "gcc_qupv3_wrap1_s2_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -586,7 +586,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s2_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s3_clk_init = {
.name = "gcc_qupv3_wrap1_s3_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -602,7 +602,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s3_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s4_clk_init = {
.name = "gcc_qupv3_wrap1_s4_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -618,7 +618,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s4_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s5_clk_init = {
.name = "gcc_qupv3_wrap1_s5_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -634,7 +634,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s5_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s6_clk_init = {
.name = "gcc_qupv3_wrap1_s6_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -650,7 +650,7 @@ static struct clk_rcg2 gcc_qupv3_wrap1_s6_clk_src = {
static struct clk_init_data gcc_qupv3_wrap1_s7_clk_init = {
.name = "gcc_qupv3_wrap1_s7_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
};
@@ -707,7 +707,7 @@ static struct clk_rcg2 gcc_sdcc4_apps_clk_src = {
.freq_tbl = ftbl_gcc_sdcc4_apps_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_sdcc4_apps_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -749,7 +749,7 @@ static struct clk_rcg2 gcc_ufs_card_axi_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_axi_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_card_axi_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -771,7 +771,7 @@ static struct clk_rcg2 gcc_ufs_card_ice_core_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_ice_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_card_ice_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -806,7 +806,7 @@ static struct clk_rcg2 gcc_ufs_card_unipro_core_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_unipro_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_card_unipro_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -829,7 +829,7 @@ static struct clk_rcg2 gcc_ufs_phy_axi_clk_src = {
.freq_tbl = ftbl_gcc_ufs_phy_axi_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_phy_axi_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -843,7 +843,7 @@ static struct clk_rcg2 gcc_ufs_phy_ice_core_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_ice_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_phy_ice_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -871,7 +871,7 @@ static struct clk_rcg2 gcc_ufs_phy_unipro_core_clk_src = {
.freq_tbl = ftbl_gcc_ufs_card_unipro_core_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_ufs_phy_unipro_core_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -894,7 +894,7 @@ static struct clk_rcg2 gcc_usb30_prim_master_clk_src = {
.freq_tbl = ftbl_gcc_usb30_prim_master_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_usb30_prim_master_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -916,7 +916,7 @@ static struct clk_rcg2 gcc_usb30_prim_mock_utmi_clk_src = {
.freq_tbl = ftbl_gcc_usb30_prim_mock_utmi_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_usb30_prim_mock_utmi_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_shared_ops,
},
@@ -930,7 +930,7 @@ static struct clk_rcg2 gcc_usb30_sec_master_clk_src = {
.freq_tbl = ftbl_gcc_usb30_prim_master_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_usb30_sec_master_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -944,7 +944,7 @@ static struct clk_rcg2 gcc_usb30_sec_mock_utmi_clk_src = {
.freq_tbl = ftbl_gcc_usb30_prim_mock_utmi_clk_src,
.clkr.hw.init = &(struct clk_init_data){
.name = "gcc_usb30_sec_mock_utmi_clk_src",
- .parent_names = gcc_parent_names_0,
+ .parent_data = gcc_parent_data_0,
.num_parents = 4,
.ops = &clk_rcg2_ops,
},
@@ -1036,8 +1036,8 @@ static struct clk_branch gcc_aggre_ufs_card_axi_clk = {
.enable_mask = BIT(0),
.hw.init = &(struct clk_init_data){
.name = "gcc_aggre_ufs_card_axi_clk",
- .parent_names = (const char *[]){
- "gcc_ufs_card_axi_clk_src",
+ .parent_hws = (const struct clk_hw *[]){
+ &gcc_ufs_card_axi_clk_src.clkr.hw
},
.num_parents = 1,
.flags = CLK_SET_RATE_PARENT,
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 8/8] arm64: dts: qcom: Specify XO clk as input to GCC node
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
` (6 preceding siblings ...)
2019-02-26 22:34 ` [PATCH v2 7/8] clk: qcom: gcc-sdm845: Migrate to DT parent mapping Stephen Boyd
@ 2019-02-26 22:34 ` Stephen Boyd
2019-03-02 0:45 ` [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
8 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-02-26 22:34 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
This is an example patch to show how DT nodes are updated to specify the
clocks which are external to the clock controller can be specified in DT
and then found by the clk core to do parent-child linkage.
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index c27cbd3bcb0a..2409241ad4ae 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -339,6 +339,8 @@
#clock-cells = <1>;
#reset-cells = <1>;
#power-domain-cells = <1>;
+ clocks = <&rpmhcc RPMH_CXO_CLK>;
+ clock-names = "xo";
};
qfprom@784000 {
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/8] Rewrite clk parent handling
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
` (7 preceding siblings ...)
2019-02-26 22:34 ` [PATCH v2 8/8] arm64: dts: qcom: Specify XO clk as input to GCC node Stephen Boyd
@ 2019-03-02 0:45 ` Stephen Boyd
8 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-03-02 0:45 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Jeffrey Hugo, Chen-Yu Tsai
Quoting Stephen Boyd (2019-02-26 14:34:21)
>
> I plan to at least merge the early patches in this series into clk-next
> so we can clear the way for the later patches. I don't think anything is
> too controversial until we get to the new way of specifying parents
> at the end and of course, the sdm845 patch is incomplete so it's not
> going to be merged.
I've pushed the first 5 to clk-next.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-02-26 22:34 ` [PATCH v2 6/8] clk: Allow parents to be specified without string names Stephen Boyd
@ 2019-03-02 18:40 ` Jerome Brunet
2019-03-06 18:00 ` Stephen Boyd
2019-03-02 21:25 ` Jeffrey Hugo
2019-03-15 10:01 ` Jerome Brunet
2 siblings, 1 reply; 18+ messages in thread
From: Jerome Brunet @ 2019-03-02 18:40 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette
Cc: linux-kernel, linux-clk, Miquel Raynal, Russell King,
Jeffrey Hugo, Chen-Yu Tsai
On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote:
> The common clk framework is lacking in ability to describe the clk
> topology without specifying strings for every possible parent-child
> link. There are a few drawbacks to the current approach:
>
> 1) String comparisons are used for everything, including describing
> topologies that are 'local' to a single clock controller.
>
> 2) clk providers (e.g. i2c clk drivers) need to create globally unique
> clk names to avoid collisions in the clk namespace, leading to awkward
> name generation code in various clk drivers.
>
> 3) DT bindings may not fully describe the clk topology and linkages
> between clk controllers because drivers can easily rely on globally unique
> strings to describe connections between clks.
>
> This leads to confusing DT bindings, complicated clk name generation
> code, and inefficient string comparisons during clk registration just so
> that the clk framework can detect the topology of the clk tree.
> Furthermore, some drivers call clk_get() and then __clk_get_name() to
> extract the globally unique clk name just so they can specify the parent
> of the clk they're registering. We have of_clk_parent_fill() but that
> mostly only works for single clks registered from a DT node, which isn't
> the norm. Let's simplify this all by introducing two new ways of
> specifying clk parents.
>
> The first method is an array of pointers to clk_hw structures
> corresponding to the parents at that index. This works for clks that are
> registered when we have access to all the clk_hw pointers for the
> parents.
>
> The second method is a mix of clk_hw pointers and strings of local and
> global parent clk names. If the .name member of the map is set we'll
> look for that clk by performing a DT based lookup of the device the clk
> is registered with and the .name specified in the map. If that fails,
> we'll fallback to the .fallback member and perform a global clk name
> lookup like we've always done before.
Nitpick: I think you forgot to update the commit message when renaming the
struct members
>
> Using either one of these new methods is entirely optional. Existing
> drivers will continue to work, and they can migrate to this new approach
> as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> array in struct clk_init_data and use one of these new methods instead.
>
Being able to specify parents from DT is great addition !!! Thx !
Overall, it looks good but with such big patch on framework is not easy get a
clear idea. I'll try to give it a spin next week.
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
> drivers/clk/clk.c | 260 ++++++++++++++++++++++++++---------
> include/linux/clk-provider.h | 19 +++
> 2 files changed, 217 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 937b8d092d17..3d01e8c56400 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list);
>
> /*** private data structures ***/
>
> +struct clk_parent_map {
> + const struct clk_hw *hw;
> + struct clk_core *core;
> + const char *fw_name;
> + const char *name;
> +};
> +
> struct clk_core {
> const char *name;
> const struct clk_ops *ops;
> @@ -46,8 +53,7 @@ struct clk_core {
> struct module *owner;
> struct device *dev;
> struct clk_core *parent;
> - const char **parent_names;
> - struct clk_core **parents;
> + struct clk_parent_map *parents;
> u8 num_parents;
> u8 new_parent_index;
> unsigned long rate;
> @@ -316,17 +322,92 @@ static struct clk_core *clk_core_lookup(const char *name)
> return NULL;
> }
>
> +/**
> + * clk_core_get - Find the parent of a clk using a clock specifier in DT
> + * @core: clk to find parent of
> + * @name: name to search for in 'clock-names' of device providing clk
> + *
> + * This is the preferred method for clk providers to find the parent of a
> + * clk when that parent is external to the clk controller. The parent_names
> + * array is indexed and treated as a local name matching a string in the device
> + * node's 'clock-names' property. This allows clk providers to use their own
> + * namespace instead of looking for a globally unique parent string.
> + *
> + * For example the following DT snippet would allow a clock registered by the
> + * clock-controller@c001 that has a clk_init_data::parent_data array
> + * with 'xtal' in the 'name' member to find the clock provided by the
> + * clock-controller@f00abcd without needing to get the globally unique name of
> + * the xtal clk.
> + *
> + * parent: clock-controller@f00abcd {
> + * reg = <0xf00abcd 0xabcd>;
> + * #clock-cells = <0>;
> + * };
> + *
> + * clock-controller@c001 {
> + * reg = <0xc001 0xf00d>;
> + * clocks = <&parent>;
> + * clock-names = "xtal";
> + * #clock-cells = <1>;
> + * };
> + *
> + * Returns: -ENOENT when the provider can't be found or the clk doesn't
> + * exist in the provider. -EINVAL when the name can't be found. NULL when the
> + * provider knows about the clk but it isn't provided on this system.
> + * A valid clk_core pointer when the clk can be found in the provider.
> + */
> +static struct clk_core *clk_core_get(struct clk_core *core, const char *name)
> +{
> + struct clk_hw *hw;
> + struct device *dev = core->dev;
> +
> + if (!dev)
> + return ERR_PTR(-ENOENT);
> +
> + /* TODO: Support clkdev clk_lookups */
> + hw = of_clk_get_hw(dev->of_node, -1, name);
> + if (IS_ERR_OR_NULL(hw))
> + return ERR_CAST(hw);
> +
> + return hw->core;
> +}
> +
> +static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
> +{
> + struct clk_parent_map *entry = &core->parents[index];
> + struct clk_core *parent = ERR_PTR(-ENOENT);
> +
> + if (entry->hw) {
> + parent = entry->hw->core;
> + /*
> + * We have a direct reference but it isn't registered yet? Orphan it
> + * and let clk_reparent() update the orphan status when the parent
> + * is registered.
> + */
> + if (!parent)
> + parent = ERR_PTR(-EPROBE_DEFER);
> + } else {
> + if (entry->fw_name)
> + parent = clk_core_get(core, entry->fw_name);
> + if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
> + parent = clk_core_lookup(entry->name);
> + }
> +
> + /* Only cache it if it's not an error */
> + if (!IS_ERR(parent))
> + entry->core = parent;
> +}
> +
> static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
> u8 index)
> {
> - if (!core || index >= core->num_parents)
> + if (!core || index >= core->num_parents || !core->parents)
> return NULL;
>
> - if (!core->parents[index])
> - core->parents[index] =
> - clk_core_lookup(core->parent_names[index]);
> + if (!core->parents[index].core)
> + clk_core_fill_parent_index(core, index);
>
> - return core->parents[index];
> + return core->parents[index].core;
> }
>
> struct clk_hw *
> @@ -1516,15 +1597,15 @@ static int clk_fetch_parent_index(struct clk_core *core,
> return -EINVAL;
>
> for (i = 0; i < core->num_parents; i++) {
> - if (core->parents[i] == parent)
> + if (core->parents[i].core == parent)
> return i;
>
> - if (core->parents[i])
> + if (core->parents[i].core)
> continue;
>
> /* Fallback to comparing globally unique names */
> - if (!strcmp(parent->name, core->parent_names[i])) {
> - core->parents[i] = parent;
> + if (!strcmp(parent->name, core->parents[i].name)) {
> + core->parents[i].core = parent;
> return i;
> }
> }
> @@ -2290,6 +2371,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
> bool clk_has_parent(struct clk *clk, struct clk *parent)
> {
> struct clk_core *core, *parent_core;
> + int i;
>
> /* NULL clocks should be nops, so return success if either is NULL. */
> if (!clk || !parent)
> @@ -2302,8 +2384,11 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
> if (core->parent == parent_core)
> return true;
>
> - return match_string(core->parent_names, core->num_parents,
> - parent_core->name) >= 0;
> + for (i = 0; i < core->num_parents; i++)
> + if (!strcmp(core->parents[i].name, parent_core->name))
> + return true;
> +
> + return false;
> }
> EXPORT_SYMBOL_GPL(clk_has_parent);
>
> @@ -2886,9 +2971,9 @@ static int possible_parents_show(struct seq_file *s, void *data)
> int i;
>
> for (i = 0; i < core->num_parents - 1; i++)
> - seq_printf(s, "%s ", core->parent_names[i]);
> + seq_printf(s, "%s ", core->parents[i].name);
>
> - seq_printf(s, "%s\n", core->parent_names[i]);
> + seq_printf(s, "%s\n", core->parents[i].name);
>
> return 0;
> }
> @@ -3022,7 +3107,7 @@ static inline void clk_debug_unregister(struct clk_core *core)
> */
> static int __clk_core_init(struct clk_core *core)
> {
> - int i, ret;
> + int ret;
> struct clk_core *orphan;
> struct hlist_node *tmp2;
> unsigned long rate;
> @@ -3076,12 +3161,6 @@ static int __clk_core_init(struct clk_core *core)
> goto out;
> }
>
> - /* throw a WARN if any entries in parent_names are NULL */
> - for (i = 0; i < core->num_parents; i++)
> - WARN(!core->parent_names[i],
> - "%s: invalid NULL in %s's .parent_names\n",
> - __func__, core->name);
> -
> core->parent = __clk_init_parent(core);
>
> /*
> @@ -3310,6 +3389,96 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
> return clk;
> }
>
> +static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
> +{
> + if (!src) {
> + if (must_exist)
> + return -EINVAL;
> + return 0;
> + }
> +
> + dst = kstrdup_const(src, GFP_KERNEL);
> + if (!dst)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int clk_core_populate_parent_map(struct clk_core *core)
> +{
> + const struct clk_init_data *init = core->hw->init;
> + u8 num_parents = init->num_parents;
> + const char * const *parent_names = init->parent_names;
> + const struct clk_hw **parent_hws = init->parent_hws;
> + const struct clk_parent_data *parent_data = init->parent_data;
> + int i, ret = 0;
> + struct clk_parent_map *parents, *parent;
> +
> + if (!num_parents)
> + return 0;
> +
> + /*
> + * Avoid unnecessary string look-ups of clk_core's possible parents by
> + * having a cache of names/clk_hw pointers to clk_core pointers.
> + */
> + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> + core->parents = parents;
> + if (!parents)
> + return -ENOMEM;
> +
> + /* Copy everything over because it might be __initdata */
> + for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> + if (parent_names) {
> + /* throw a WARN if any entries are NULL */
> + WARN(!parent_names[i],
> + "%s: invalid NULL in %s's .parent_names\n",
> + __func__, core->name);
> + ret = clk_cpy_name(parent->name, parent_names[i],
> + true);
> + } else if (parent_data) {
> + parent->hw = parent_data[i].hw;
> + ret = clk_cpy_name(parent->fw_name,
> + parent_data[i].fw_name, false);
> + if (!ret)
> + ret = clk_cpy_name(parent->name,
> + parent_data[i].name,
> + false);
> + } else if (parent_hws) {
> + parent->hw = parent_hws[i];
> + } else {
> + ret = -EINVAL;
> + WARN(1, "Must specify parents if num_parents > 0\n");
> + }
> +
> + if (ret) {
> + do {
> + kfree_const(parents[i].name);
> + kfree_const(parents[i].fw_name);
> + } while (--i >= 0);
> + kfree(parents);
> +
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void clk_core_free_parent_map(struct clk_core *core)
> +{
> + int i = core->num_parents;
> +
> + if (!core->num_parents)
> + return;
> +
> + while (--i >= 0) {
> + kfree_const(core->parents[i].name);
> + kfree_const(core->parents[i].fw_name);
> + }
> +
> + kfree(core->parents);
> +}
> +
> /**
> * clk_register - allocate a new clock, register it and return an opaque cookie
> * @dev: device that is registering this clock
> @@ -3323,7 +3492,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
> */
> struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> {
> - int i, ret;
> + int ret;
> struct clk_core *core;
>
> core = kzalloc(sizeof(*core), GFP_KERNEL);
> @@ -3356,33 +3525,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> core->max_rate = ULONG_MAX;
> hw->core = core;
>
> - /* allocate local copy in case parent_names is __initdata */
> - core->parent_names = kcalloc(core->num_parents, sizeof(char *),
> - GFP_KERNEL);
> -
> - if (!core->parent_names) {
> - ret = -ENOMEM;
> - goto fail_parent_names;
> - }
> -
> -
> - /* copy each string name in case parent_names is __initdata */
> - for (i = 0; i < core->num_parents; i++) {
> - core->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
> - GFP_KERNEL);
> - if (!core->parent_names[i]) {
> - ret = -ENOMEM;
> - goto fail_parent_names_copy;
> - }
> - }
> -
> - /* avoid unnecessary string look-ups of clk_core's possible parents. */
> - core->parents = kcalloc(core->num_parents, sizeof(*core->parents),
> - GFP_KERNEL);
> - if (!core->parents) {
> - ret = -ENOMEM;
> + ret = clk_core_populate_parent_map(core);
> + if (ret)
> goto fail_parents;
> - };
>
> INIT_HLIST_HEAD(&core->clks);
>
> @@ -3393,7 +3538,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> hw->clk = alloc_clk(core, NULL, NULL);
> if (IS_ERR(hw->clk)) {
> ret = PTR_ERR(hw->clk);
> - goto fail_parents;
> + goto fail_create_clk;
> }
>
> clk_core_link_consumer(hw->core, hw->clk);
> @@ -3409,13 +3554,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> free_clk(hw->clk);
> hw->clk = NULL;
>
> +fail_create_clk:
> + clk_core_free_parent_map(core);
> fail_parents:
> - kfree(core->parents);
> -fail_parent_names_copy:
> - while (--i >= 0)
> - kfree_const(core->parent_names[i]);
> - kfree(core->parent_names);
> -fail_parent_names:
> fail_ops:
> kfree_const(core->name);
> fail_name:
> @@ -3445,15 +3586,10 @@ EXPORT_SYMBOL_GPL(clk_hw_register);
> static void __clk_release(struct kref *ref)
> {
> struct clk_core *core = container_of(ref, struct clk_core, ref);
> - int i = core->num_parents;
>
> lockdep_assert_held(&prepare_lock);
>
> - kfree(core->parents);
> - while (--i >= 0)
> - kfree_const(core->parent_names[i]);
> -
> - kfree(core->parent_names);
> + clk_core_free_parent_map(core);
> kfree_const(core->name);
> kfree(core);
> }
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index e443fa9fa859..f4de52c6764e 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -250,6 +250,18 @@ struct clk_ops {
> void (*debug_init)(struct clk_hw *hw, struct dentry *dentry);
> };
>
> +/**
> + * struct clk_parent_data - clk parent information
> + * @hw: parent clk_hw pointer (used for clk providers with internal clks)
> + * @fw_name: parent name local to provider registering clk
> + * @name: globally unique parent name (used as a fallback)
> + */
> +struct clk_parent_data {
> + const struct clk_hw *hw;
> + const char *fw_name;
> + const char *name;
> +};
> +
> /**
> * struct clk_init_data - holds init data that's common to all clocks and is
> * shared between the clock provider and the common clock framework.
> @@ -257,13 +269,20 @@ struct clk_ops {
> * @name: clock name
> * @ops: operations this clock supports
> * @parent_names: array of string names for all possible parents
> + * @parent_data: array of parent data for all possible parents (when some
> + * parents are external to the clk controller)
> + * @parent_hws: array of pointers to all possible parents (when all parents
> + * are internal to the clk controller)
> * @num_parents: number of possible parents
> * @flags: framework-level hints and quirks
> */
> struct clk_init_data {
> const char *name;
> const struct clk_ops *ops;
> + /* Only one of the following three should be assigned */
> const char * const *parent_names;
> + const struct clk_parent_data *parent_data;
> + const struct clk_hw **parent_hws;
> u8 num_parents;
> unsigned long flags;
> };
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-02-26 22:34 ` [PATCH v2 6/8] clk: Allow parents to be specified without string names Stephen Boyd
2019-03-02 18:40 ` Jerome Brunet
@ 2019-03-02 21:25 ` Jeffrey Hugo
2019-03-06 17:48 ` Stephen Boyd
2019-03-15 10:01 ` Jerome Brunet
2 siblings, 1 reply; 18+ messages in thread
From: Jeffrey Hugo @ 2019-03-02 21:25 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Chen-Yu Tsai
On 2/26/2019 3:34 PM, Stephen Boyd wrote:
> The common clk framework is lacking in ability to describe the clk
> topology without specifying strings for every possible parent-child
> link. There are a few drawbacks to the current approach:
>
> 1) String comparisons are used for everything, including describing
> topologies that are 'local' to a single clock controller.
>
> 2) clk providers (e.g. i2c clk drivers) need to create globally unique
> clk names to avoid collisions in the clk namespace, leading to awkward
> name generation code in various clk drivers.
>
> 3) DT bindings may not fully describe the clk topology and linkages
> between clk controllers because drivers can easily rely on globally unique
> strings to describe connections between clks.
>
> This leads to confusing DT bindings, complicated clk name generation
> code, and inefficient string comparisons during clk registration just so
> that the clk framework can detect the topology of the clk tree.
> Furthermore, some drivers call clk_get() and then __clk_get_name() to
> extract the globally unique clk name just so they can specify the parent
> of the clk they're registering. We have of_clk_parent_fill() but that
> mostly only works for single clks registered from a DT node, which isn't
> the norm. Let's simplify this all by introducing two new ways of
> specifying clk parents.
>
> The first method is an array of pointers to clk_hw structures
> corresponding to the parents at that index. This works for clks that are
> registered when we have access to all the clk_hw pointers for the
> parents.
>
> The second method is a mix of clk_hw pointers and strings of local and
> global parent clk names. If the .name member of the map is set we'll
> look for that clk by performing a DT based lookup of the device the clk
> is registered with and the .name specified in the map. If that fails,
> we'll fallback to the .fallback member and perform a global clk name
> lookup like we've always done before.
>
> Using either one of these new methods is entirely optional. Existing
> drivers will continue to work, and they can migrate to this new approach
> as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> array in struct clk_init_data and use one of these new methods instead.
>
I don't know exactly what regressed from V1, but this change breaks all
clock drivers as far as I can tell. All clocks from old and new (ie
8998 mmcc rebased onto this) drivers end up as orphans.
Is there some data I can provide to help you figure out the issue?
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
> drivers/clk/clk.c | 260 ++++++++++++++++++++++++++---------
> include/linux/clk-provider.h | 19 +++
> 2 files changed, 217 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 937b8d092d17..3d01e8c56400 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list);
>
> /*** private data structures ***/
>
> +struct clk_parent_map {
> + const struct clk_hw *hw;
> + struct clk_core *core;
> + const char *fw_name;
> + const char *name;
> +};
> +
> struct clk_core {
> const char *name;
> const struct clk_ops *ops;
> @@ -46,8 +53,7 @@ struct clk_core {
> struct module *owner;
> struct device *dev;
> struct clk_core *parent;
> - const char **parent_names;
> - struct clk_core **parents;
> + struct clk_parent_map *parents;
> u8 num_parents;
> u8 new_parent_index;
> unsigned long rate;
> @@ -316,17 +322,92 @@ static struct clk_core *clk_core_lookup(const char *name)
> return NULL;
> }
>
> +/**
> + * clk_core_get - Find the parent of a clk using a clock specifier in DT
> + * @core: clk to find parent of
> + * @name: name to search for in 'clock-names' of device providing clk
> + *
> + * This is the preferred method for clk providers to find the parent of a
> + * clk when that parent is external to the clk controller. The parent_names
> + * array is indexed and treated as a local name matching a string in the device
> + * node's 'clock-names' property. This allows clk providers to use their own
> + * namespace instead of looking for a globally unique parent string.
> + *
> + * For example the following DT snippet would allow a clock registered by the
> + * clock-controller@c001 that has a clk_init_data::parent_data array
> + * with 'xtal' in the 'name' member to find the clock provided by the
> + * clock-controller@f00abcd without needing to get the globally unique name of
> + * the xtal clk.
> + *
> + * parent: clock-controller@f00abcd {
> + * reg = <0xf00abcd 0xabcd>;
> + * #clock-cells = <0>;
> + * };
> + *
> + * clock-controller@c001 {
> + * reg = <0xc001 0xf00d>;
> + * clocks = <&parent>;
> + * clock-names = "xtal";
> + * #clock-cells = <1>;
> + * };
> + *
> + * Returns: -ENOENT when the provider can't be found or the clk doesn't
> + * exist in the provider. -EINVAL when the name can't be found. NULL when the
> + * provider knows about the clk but it isn't provided on this system.
> + * A valid clk_core pointer when the clk can be found in the provider.
> + */
> +static struct clk_core *clk_core_get(struct clk_core *core, const char *name)
> +{
> + struct clk_hw *hw;
> + struct device *dev = core->dev;
> +
> + if (!dev)
> + return ERR_PTR(-ENOENT);
> +
> + /* TODO: Support clkdev clk_lookups */
> + hw = of_clk_get_hw(dev->of_node, -1, name);
> + if (IS_ERR_OR_NULL(hw))
> + return ERR_CAST(hw);
> +
> + return hw->core;
> +}
> +
> +static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
> +{
> + struct clk_parent_map *entry = &core->parents[index];
> + struct clk_core *parent = ERR_PTR(-ENOENT);
> +
> + if (entry->hw) {
> + parent = entry->hw->core;
> + /*
> + * We have a direct reference but it isn't registered yet? Orphan it
> + * and let clk_reparent() update the orphan status when the parent
> + * is registered.
> + */
> + if (!parent)
> + parent = ERR_PTR(-EPROBE_DEFER);
> + } else {
> + if (entry->fw_name)
> + parent = clk_core_get(core, entry->fw_name);
> + if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
> + parent = clk_core_lookup(entry->name);
> + }
> +
> + /* Only cache it if it's not an error */
> + if (!IS_ERR(parent))
> + entry->core = parent;
> +}
> +
> static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
> u8 index)
> {
> - if (!core || index >= core->num_parents)
> + if (!core || index >= core->num_parents || !core->parents)
> return NULL;
>
> - if (!core->parents[index])
> - core->parents[index] =
> - clk_core_lookup(core->parent_names[index]);
> + if (!core->parents[index].core)
> + clk_core_fill_parent_index(core, index);
>
> - return core->parents[index];
> + return core->parents[index].core;
> }
>
> struct clk_hw *
> @@ -1516,15 +1597,15 @@ static int clk_fetch_parent_index(struct clk_core *core,
> return -EINVAL;
>
> for (i = 0; i < core->num_parents; i++) {
> - if (core->parents[i] == parent)
> + if (core->parents[i].core == parent)
> return i;
>
> - if (core->parents[i])
> + if (core->parents[i].core)
> continue;
>
> /* Fallback to comparing globally unique names */
> - if (!strcmp(parent->name, core->parent_names[i])) {
> - core->parents[i] = parent;
> + if (!strcmp(parent->name, core->parents[i].name)) {
> + core->parents[i].core = parent;
> return i;
> }
> }
> @@ -2290,6 +2371,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
> bool clk_has_parent(struct clk *clk, struct clk *parent)
> {
> struct clk_core *core, *parent_core;
> + int i;
>
> /* NULL clocks should be nops, so return success if either is NULL. */
> if (!clk || !parent)
> @@ -2302,8 +2384,11 @@ bool clk_has_parent(struct clk *clk, struct clk *parent)
> if (core->parent == parent_core)
> return true;
>
> - return match_string(core->parent_names, core->num_parents,
> - parent_core->name) >= 0;
> + for (i = 0; i < core->num_parents; i++)
> + if (!strcmp(core->parents[i].name, parent_core->name))
> + return true;
> +
> + return false;
> }
> EXPORT_SYMBOL_GPL(clk_has_parent);
>
> @@ -2886,9 +2971,9 @@ static int possible_parents_show(struct seq_file *s, void *data)
> int i;
>
> for (i = 0; i < core->num_parents - 1; i++)
> - seq_printf(s, "%s ", core->parent_names[i]);
> + seq_printf(s, "%s ", core->parents[i].name);
>
> - seq_printf(s, "%s\n", core->parent_names[i]);
> + seq_printf(s, "%s\n", core->parents[i].name);
>
> return 0;
> }
> @@ -3022,7 +3107,7 @@ static inline void clk_debug_unregister(struct clk_core *core)
> */
> static int __clk_core_init(struct clk_core *core)
> {
> - int i, ret;
> + int ret;
> struct clk_core *orphan;
> struct hlist_node *tmp2;
> unsigned long rate;
> @@ -3076,12 +3161,6 @@ static int __clk_core_init(struct clk_core *core)
> goto out;
> }
>
> - /* throw a WARN if any entries in parent_names are NULL */
> - for (i = 0; i < core->num_parents; i++)
> - WARN(!core->parent_names[i],
> - "%s: invalid NULL in %s's .parent_names\n",
> - __func__, core->name);
> -
> core->parent = __clk_init_parent(core);
>
> /*
> @@ -3310,6 +3389,96 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
> return clk;
> }
>
> +static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
> +{
> + if (!src) {
> + if (must_exist)
> + return -EINVAL;
> + return 0;
> + }
> +
> + dst = kstrdup_const(src, GFP_KERNEL);
> + if (!dst)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int clk_core_populate_parent_map(struct clk_core *core)
> +{
> + const struct clk_init_data *init = core->hw->init;
> + u8 num_parents = init->num_parents;
> + const char * const *parent_names = init->parent_names;
> + const struct clk_hw **parent_hws = init->parent_hws;
> + const struct clk_parent_data *parent_data = init->parent_data;
> + int i, ret = 0;
> + struct clk_parent_map *parents, *parent;
> +
> + if (!num_parents)
> + return 0;
> +
> + /*
> + * Avoid unnecessary string look-ups of clk_core's possible parents by
> + * having a cache of names/clk_hw pointers to clk_core pointers.
> + */
> + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> + core->parents = parents;
> + if (!parents)
> + return -ENOMEM;
> +
> + /* Copy everything over because it might be __initdata */
> + for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> + if (parent_names) {
> + /* throw a WARN if any entries are NULL */
> + WARN(!parent_names[i],
> + "%s: invalid NULL in %s's .parent_names\n",
> + __func__, core->name);
> + ret = clk_cpy_name(parent->name, parent_names[i],
> + true);
> + } else if (parent_data) {
> + parent->hw = parent_data[i].hw;
> + ret = clk_cpy_name(parent->fw_name,
> + parent_data[i].fw_name, false);
> + if (!ret)
> + ret = clk_cpy_name(parent->name,
> + parent_data[i].name,
> + false);
> + } else if (parent_hws) {
> + parent->hw = parent_hws[i];
> + } else {
> + ret = -EINVAL;
> + WARN(1, "Must specify parents if num_parents > 0\n");
> + }
> +
> + if (ret) {
> + do {
> + kfree_const(parents[i].name);
> + kfree_const(parents[i].fw_name);
> + } while (--i >= 0);
> + kfree(parents);
> +
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void clk_core_free_parent_map(struct clk_core *core)
> +{
> + int i = core->num_parents;
> +
> + if (!core->num_parents)
> + return;
> +
> + while (--i >= 0) {
> + kfree_const(core->parents[i].name);
> + kfree_const(core->parents[i].fw_name);
> + }
> +
> + kfree(core->parents);
> +}
> +
> /**
> * clk_register - allocate a new clock, register it and return an opaque cookie
> * @dev: device that is registering this clock
> @@ -3323,7 +3492,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
> */
> struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> {
> - int i, ret;
> + int ret;
> struct clk_core *core;
>
> core = kzalloc(sizeof(*core), GFP_KERNEL);
> @@ -3356,33 +3525,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> core->max_rate = ULONG_MAX;
> hw->core = core;
>
> - /* allocate local copy in case parent_names is __initdata */
> - core->parent_names = kcalloc(core->num_parents, sizeof(char *),
> - GFP_KERNEL);
> -
> - if (!core->parent_names) {
> - ret = -ENOMEM;
> - goto fail_parent_names;
> - }
> -
> -
> - /* copy each string name in case parent_names is __initdata */
> - for (i = 0; i < core->num_parents; i++) {
> - core->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
> - GFP_KERNEL);
> - if (!core->parent_names[i]) {
> - ret = -ENOMEM;
> - goto fail_parent_names_copy;
> - }
> - }
> -
> - /* avoid unnecessary string look-ups of clk_core's possible parents. */
> - core->parents = kcalloc(core->num_parents, sizeof(*core->parents),
> - GFP_KERNEL);
> - if (!core->parents) {
> - ret = -ENOMEM;
> + ret = clk_core_populate_parent_map(core);
> + if (ret)
> goto fail_parents;
> - };
>
> INIT_HLIST_HEAD(&core->clks);
>
> @@ -3393,7 +3538,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> hw->clk = alloc_clk(core, NULL, NULL);
> if (IS_ERR(hw->clk)) {
> ret = PTR_ERR(hw->clk);
> - goto fail_parents;
> + goto fail_create_clk;
> }
>
> clk_core_link_consumer(hw->core, hw->clk);
> @@ -3409,13 +3554,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> free_clk(hw->clk);
> hw->clk = NULL;
>
> +fail_create_clk:
> + clk_core_free_parent_map(core);
> fail_parents:
> - kfree(core->parents);
> -fail_parent_names_copy:
> - while (--i >= 0)
> - kfree_const(core->parent_names[i]);
> - kfree(core->parent_names);
> -fail_parent_names:
> fail_ops:
> kfree_const(core->name);
> fail_name:
> @@ -3445,15 +3586,10 @@ EXPORT_SYMBOL_GPL(clk_hw_register);
> static void __clk_release(struct kref *ref)
> {
> struct clk_core *core = container_of(ref, struct clk_core, ref);
> - int i = core->num_parents;
>
> lockdep_assert_held(&prepare_lock);
>
> - kfree(core->parents);
> - while (--i >= 0)
> - kfree_const(core->parent_names[i]);
> -
> - kfree(core->parent_names);
> + clk_core_free_parent_map(core);
> kfree_const(core->name);
> kfree(core);
> }
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index e443fa9fa859..f4de52c6764e 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -250,6 +250,18 @@ struct clk_ops {
> void (*debug_init)(struct clk_hw *hw, struct dentry *dentry);
> };
>
> +/**
> + * struct clk_parent_data - clk parent information
> + * @hw: parent clk_hw pointer (used for clk providers with internal clks)
> + * @fw_name: parent name local to provider registering clk
> + * @name: globally unique parent name (used as a fallback)
> + */
> +struct clk_parent_data {
> + const struct clk_hw *hw;
> + const char *fw_name;
> + const char *name;
> +};
> +
> /**
> * struct clk_init_data - holds init data that's common to all clocks and is
> * shared between the clock provider and the common clock framework.
> @@ -257,13 +269,20 @@ struct clk_ops {
> * @name: clock name
> * @ops: operations this clock supports
> * @parent_names: array of string names for all possible parents
> + * @parent_data: array of parent data for all possible parents (when some
> + * parents are external to the clk controller)
> + * @parent_hws: array of pointers to all possible parents (when all parents
> + * are internal to the clk controller)
> * @num_parents: number of possible parents
> * @flags: framework-level hints and quirks
> */
> struct clk_init_data {
> const char *name;
> const struct clk_ops *ops;
> + /* Only one of the following three should be assigned */
> const char * const *parent_names;
> + const struct clk_parent_data *parent_data;
> + const struct clk_hw **parent_hws;
> u8 num_parents;
> unsigned long flags;
> };
>
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-03-02 21:25 ` Jeffrey Hugo
@ 2019-03-06 17:48 ` Stephen Boyd
2019-03-06 21:45 ` Jeffrey Hugo
0 siblings, 1 reply; 18+ messages in thread
From: Stephen Boyd @ 2019-03-06 17:48 UTC (permalink / raw)
To: Jeffrey Hugo, Michael Turquette
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Chen-Yu Tsai
Quoting Jeffrey Hugo (2019-03-02 13:25:06)
> On 2/26/2019 3:34 PM, Stephen Boyd wrote:
> > The common clk framework is lacking in ability to describe the clk
> > topology without specifying strings for every possible parent-child
> > link. There are a few drawbacks to the current approach:
> >
> > 1) String comparisons are used for everything, including describing
> > topologies that are 'local' to a single clock controller.
> >
> > 2) clk providers (e.g. i2c clk drivers) need to create globally unique
> > clk names to avoid collisions in the clk namespace, leading to awkward
> > name generation code in various clk drivers.
> >
> > 3) DT bindings may not fully describe the clk topology and linkages
> > between clk controllers because drivers can easily rely on globally unique
> > strings to describe connections between clks.
> >
> > This leads to confusing DT bindings, complicated clk name generation
> > code, and inefficient string comparisons during clk registration just so
> > that the clk framework can detect the topology of the clk tree.
> > Furthermore, some drivers call clk_get() and then __clk_get_name() to
> > extract the globally unique clk name just so they can specify the parent
> > of the clk they're registering. We have of_clk_parent_fill() but that
> > mostly only works for single clks registered from a DT node, which isn't
> > the norm. Let's simplify this all by introducing two new ways of
> > specifying clk parents.
> >
> > The first method is an array of pointers to clk_hw structures
> > corresponding to the parents at that index. This works for clks that are
> > registered when we have access to all the clk_hw pointers for the
> > parents.
> >
> > The second method is a mix of clk_hw pointers and strings of local and
> > global parent clk names. If the .name member of the map is set we'll
> > look for that clk by performing a DT based lookup of the device the clk
> > is registered with and the .name specified in the map. If that fails,
> > we'll fallback to the .fallback member and perform a global clk name
> > lookup like we've always done before.
> >
> > Using either one of these new methods is entirely optional. Existing
> > drivers will continue to work, and they can migrate to this new approach
> > as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> > array in struct clk_init_data and use one of these new methods instead.
> >
>
> I don't know exactly what regressed from V1, but this change breaks all
> clock drivers as far as I can tell. All clocks from old and new (ie
> 8998 mmcc rebased onto this) drivers end up as orphans.
>
> Is there some data I can provide to help you figure out the issue?
>
Can you try this patch? It fixes a pointer blunder that I'm sad about.
----8<-----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 37aea7884166..d12afd256dc5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3297,15 +3297,17 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
return clk;
}
-static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
+static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
{
+ const char *dst;
+
if (!src) {
if (must_exist)
return -EINVAL;
return 0;
}
- dst = kstrdup_const(src, GFP_KERNEL);
+ *dst_p = dst = kstrdup_const(src, GFP_KERNEL);
if (!dst)
return -ENOMEM;
@@ -3341,14 +3343,14 @@ static int clk_core_populate_parent_map(struct clk_core *core)
WARN(!parent_names[i],
"%s: invalid NULL in %s's .parent_names\n",
__func__, core->name);
- ret = clk_cpy_name(parent->name, parent_names[i],
+ ret = clk_cpy_name(&parent->name, parent_names[i],
true);
} else if (parent_data) {
parent->hw = parent_data[i].hw;
- ret = clk_cpy_name(parent->fw_name,
+ ret = clk_cpy_name(&parent->fw_name,
parent_data[i].fw_name, false);
if (!ret)
- ret = clk_cpy_name(parent->name,
+ ret = clk_cpy_name(&parent->name,
parent_data[i].name,
false);
} else if (parent_hws) {
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-03-02 18:40 ` Jerome Brunet
@ 2019-03-06 18:00 ` Stephen Boyd
0 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2019-03-06 18:00 UTC (permalink / raw)
To: Jerome Brunet, Michael Turquette
Cc: linux-kernel, linux-clk, Miquel Raynal, Russell King,
Jeffrey Hugo, Chen-Yu Tsai
Quoting Jerome Brunet (2019-03-02 10:40:42)
> On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote:
> > The common clk framework is lacking in ability to describe the clk
> > topology without specifying strings for every possible parent-child
> > link. There are a few drawbacks to the current approach:
> >
> > 1) String comparisons are used for everything, including describing
> > topologies that are 'local' to a single clock controller.
> >
> > 2) clk providers (e.g. i2c clk drivers) need to create globally unique
> > clk names to avoid collisions in the clk namespace, leading to awkward
> > name generation code in various clk drivers.
> >
> > 3) DT bindings may not fully describe the clk topology and linkages
> > between clk controllers because drivers can easily rely on globally unique
> > strings to describe connections between clks.
> >
> > This leads to confusing DT bindings, complicated clk name generation
> > code, and inefficient string comparisons during clk registration just so
> > that the clk framework can detect the topology of the clk tree.
> > Furthermore, some drivers call clk_get() and then __clk_get_name() to
> > extract the globally unique clk name just so they can specify the parent
> > of the clk they're registering. We have of_clk_parent_fill() but that
> > mostly only works for single clks registered from a DT node, which isn't
> > the norm. Let's simplify this all by introducing two new ways of
> > specifying clk parents.
> >
> > The first method is an array of pointers to clk_hw structures
> > corresponding to the parents at that index. This works for clks that are
> > registered when we have access to all the clk_hw pointers for the
> > parents.
> >
> > The second method is a mix of clk_hw pointers and strings of local and
> > global parent clk names. If the .name member of the map is set we'll
> > look for that clk by performing a DT based lookup of the device the clk
> > is registered with and the .name specified in the map. If that fails,
> > we'll fallback to the .fallback member and perform a global clk name
> > lookup like we've always done before.
>
> Nitpick: I think you forgot to update the commit message when renaming the
> struct members
Fixed. Thanks!
>
> >
> > Using either one of these new methods is entirely optional. Existing
> > drivers will continue to work, and they can migrate to this new approach
> > as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> > array in struct clk_init_data and use one of these new methods instead.
> >
>
> Being able to specify parents from DT is great addition !!! Thx !
Woohooo!
>
> Overall, it looks good but with such big patch on framework is not easy get a
> clear idea. I'll try to give it a spin next week.
Ok. Please be aware that I found one bug already but there are probably
more lurking. I've also pushed the branch out to clk.git on kernel.org
if you're interested in looking at the mass conversions going on.
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-parent-rewrite
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-03-06 17:48 ` Stephen Boyd
@ 2019-03-06 21:45 ` Jeffrey Hugo
0 siblings, 0 replies; 18+ messages in thread
From: Jeffrey Hugo @ 2019-03-06 21:45 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette
Cc: linux-kernel, linux-clk, Miquel Raynal, Jerome Brunet,
Russell King, Chen-Yu Tsai
On 3/6/2019 10:48 AM, Stephen Boyd wrote:
> Quoting Jeffrey Hugo (2019-03-02 13:25:06)
>> On 2/26/2019 3:34 PM, Stephen Boyd wrote:
>>> The common clk framework is lacking in ability to describe the clk
>>> topology without specifying strings for every possible parent-child
>>> link. There are a few drawbacks to the current approach:
>>>
>>> 1) String comparisons are used for everything, including describing
>>> topologies that are 'local' to a single clock controller.
>>>
>>> 2) clk providers (e.g. i2c clk drivers) need to create globally unique
>>> clk names to avoid collisions in the clk namespace, leading to awkward
>>> name generation code in various clk drivers.
>>>
>>> 3) DT bindings may not fully describe the clk topology and linkages
>>> between clk controllers because drivers can easily rely on globally unique
>>> strings to describe connections between clks.
>>>
>>> This leads to confusing DT bindings, complicated clk name generation
>>> code, and inefficient string comparisons during clk registration just so
>>> that the clk framework can detect the topology of the clk tree.
>>> Furthermore, some drivers call clk_get() and then __clk_get_name() to
>>> extract the globally unique clk name just so they can specify the parent
>>> of the clk they're registering. We have of_clk_parent_fill() but that
>>> mostly only works for single clks registered from a DT node, which isn't
>>> the norm. Let's simplify this all by introducing two new ways of
>>> specifying clk parents.
>>>
>>> The first method is an array of pointers to clk_hw structures
>>> corresponding to the parents at that index. This works for clks that are
>>> registered when we have access to all the clk_hw pointers for the
>>> parents.
>>>
>>> The second method is a mix of clk_hw pointers and strings of local and
>>> global parent clk names. If the .name member of the map is set we'll
>>> look for that clk by performing a DT based lookup of the device the clk
>>> is registered with and the .name specified in the map. If that fails,
>>> we'll fallback to the .fallback member and perform a global clk name
>>> lookup like we've always done before.
>>>
>>> Using either one of these new methods is entirely optional. Existing
>>> drivers will continue to work, and they can migrate to this new approach
>>> as they see fit. Eventually, we'll want to get rid of the 'parent_names'
>>> array in struct clk_init_data and use one of these new methods instead.
>>>
>>
>> I don't know exactly what regressed from V1, but this change breaks all
>> clock drivers as far as I can tell. All clocks from old and new (ie
>> 8998 mmcc rebased onto this) drivers end up as orphans.
>>
>> Is there some data I can provide to help you figure out the issue?
>>
>
> Can you try this patch? It fixes a pointer blunder that I'm sad about.
That did the trick. Everything seems to work again. I haven't
identified any additional issues.
>
> ----8<-----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 37aea7884166..d12afd256dc5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3297,15 +3297,17 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
> return clk;
> }
>
> -static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
> +static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
> {
> + const char *dst;
> +
> if (!src) {
> if (must_exist)
> return -EINVAL;
> return 0;
> }
>
> - dst = kstrdup_const(src, GFP_KERNEL);
> + *dst_p = dst = kstrdup_const(src, GFP_KERNEL);
> if (!dst)
> return -ENOMEM;
>
> @@ -3341,14 +3343,14 @@ static int clk_core_populate_parent_map(struct clk_core *core)
> WARN(!parent_names[i],
> "%s: invalid NULL in %s's .parent_names\n",
> __func__, core->name);
> - ret = clk_cpy_name(parent->name, parent_names[i],
> + ret = clk_cpy_name(&parent->name, parent_names[i],
> true);
> } else if (parent_data) {
> parent->hw = parent_data[i].hw;
> - ret = clk_cpy_name(parent->fw_name,
> + ret = clk_cpy_name(&parent->fw_name,
> parent_data[i].fw_name, false);
> if (!ret)
> - ret = clk_cpy_name(parent->name,
> + ret = clk_cpy_name(&parent->name,
> parent_data[i].name,
> false);
> } else if (parent_hws) {
>
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-02-26 22:34 ` [PATCH v2 6/8] clk: Allow parents to be specified without string names Stephen Boyd
2019-03-02 18:40 ` Jerome Brunet
2019-03-02 21:25 ` Jeffrey Hugo
@ 2019-03-15 10:01 ` Jerome Brunet
2019-03-15 17:16 ` Stephen Boyd
2 siblings, 1 reply; 18+ messages in thread
From: Jerome Brunet @ 2019-03-15 10:01 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette
Cc: linux-kernel, linux-clk, Miquel Raynal, Russell King,
Jeffrey Hugo, Chen-Yu Tsai
On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote:
> The common clk framework is lacking in ability to describe the clk
> topology without specifying strings for every possible parent-child
> link. There are a few drawbacks to the current approach:
>
> 1) String comparisons are used for everything, including describing
> topologies that are 'local' to a single clock controller.
>
> 2) clk providers (e.g. i2c clk drivers) need to create globally unique
> clk names to avoid collisions in the clk namespace, leading to awkward
> name generation code in various clk drivers.
>
> 3) DT bindings may not fully describe the clk topology and linkages
> between clk controllers because drivers can easily rely on globally unique
> strings to describe connections between clks.
>
> This leads to confusing DT bindings, complicated clk name generation
> code, and inefficient string comparisons during clk registration just so
> that the clk framework can detect the topology of the clk tree.
> Furthermore, some drivers call clk_get() and then __clk_get_name() to
> extract the globally unique clk name just so they can specify the parent
> of the clk they're registering. We have of_clk_parent_fill() but that
> mostly only works for single clks registered from a DT node, which isn't
> the norm. Let's simplify this all by introducing two new ways of
> specifying clk parents.
>
> The first method is an array of pointers to clk_hw structures
> corresponding to the parents at that index. This works for clks that are
> registered when we have access to all the clk_hw pointers for the
> parents.
>
> The second method is a mix of clk_hw pointers and strings of local and
> global parent clk names. If the .name member of the map is set we'll
> look for that clk by performing a DT based lookup of the device the clk
> is registered with and the .name specified in the map. If that fails,
> we'll fallback to the .fallback member and perform a global clk name
> lookup like we've always done before.
>
> Using either one of these new methods is entirely optional. Existing
> drivers will continue to work, and they can migrate to this new approach
> as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> array in struct clk_init_data and use one of these new methods instead.
>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
> drivers/clk/clk.c | 260 ++++++++++++++++++++++++++---------
> include/linux/clk-provider.h | 19 +++
> 2 files changed, 217 insertions(+), 62 deletions(-)
Sorry for the delay.
With the fix you sent to Jeffrey
Tested by porting the aoclk controller of Amlogic g12a SoC.
This allowed to test
* hws only table
* parent_data with a mix of hw pointers and fw_name (with different input
controllers and also an input that is optional and never provided)
Tested-by: Jerome Brunet <jbrunet@baylibre.com>
With the small comment below
Reviewed-by Jerome Brunet <jbrunet@baylibre.com>
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 937b8d092d17..3d01e8c56400 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list);
>
[...]
>
> +static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
> +{
> + if (!src) {
> + if (must_exist)
> + return -EINVAL;
> + return 0;
> + }
> +
> + dst = kstrdup_const(src, GFP_KERNEL);
> + if (!dst)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int clk_core_populate_parent_map(struct clk_core *core)
> +{
> + const struct clk_init_data *init = core->hw->init;
> + u8 num_parents = init->num_parents;
> + const char * const *parent_names = init->parent_names;
> + const struct clk_hw **parent_hws = init->parent_hws;
> + const struct clk_parent_data *parent_data = init->parent_data;
> + int i, ret = 0;
> + struct clk_parent_map *parents, *parent;
> +
> + if (!num_parents)
> + return 0;
> +
> + /*
> + * Avoid unnecessary string look-ups of clk_core's possible parents by
> + * having a cache of names/clk_hw pointers to clk_core pointers.
> + */
> + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> + core->parents = parents;
> + if (!parents)
> + return -ENOMEM;
> +
> + /* Copy everything over because it might be __initdata */
> + for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> + if (parent_names) {
> + /* throw a WARN if any entries are NULL */
> + WARN(!parent_names[i],
> + "%s: invalid NULL in %s's .parent_names\n",
> + __func__, core->name);
> + ret = clk_cpy_name(parent->name, parent_names[i],
> + true);
> + } else if (parent_data) {
While testing, I mistakenly left both parent_names and parent_data. I was
surprised that parent_data did not take precedence of parent_names.
Maybe it should ? (... but I understand we are not supposed to provide both)
> + parent->hw = parent_data[i].hw;
> + ret = clk_cpy_name(parent->fw_name,
> + parent_data[i].fw_name, false);
> + if (!ret)
> + ret = clk_cpy_name(parent->name,
> + parent_data[i].name,
> + false);
> + } else if (parent_hws) {
> + parent->hw = parent_hws[i];
> + } else {
Maybe there should also some kinda of check to verify if more than one option
(among hws, parent_data and parent_names) was provided and throw a warn ?
Could be useful with drivers move away from parent_names ?
> + ret = -EINVAL;
> + WARN(1, "Must specify parents if num_parents > 0\n");
> + }
> +
> + if (ret) {
> + do {
> + kfree_const(parents[i].name);
> + kfree_const(parents[i].fw_name);
> + } while (--i >= 0);
> + kfree(parents);
> +
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-03-15 10:01 ` Jerome Brunet
@ 2019-03-15 17:16 ` Stephen Boyd
2019-03-19 9:25 ` Jerome Brunet
0 siblings, 1 reply; 18+ messages in thread
From: Stephen Boyd @ 2019-03-15 17:16 UTC (permalink / raw)
To: Jerome Brunet, Michael Turquette
Cc: linux-kernel, linux-clk, Miquel Raynal, Russell King,
Jeffrey Hugo, Chen-Yu Tsai
Quoting Jerome Brunet (2019-03-15 03:01:53)
> On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote:
> > ---
> > drivers/clk/clk.c | 260 ++++++++++++++++++++++++++---------
> > include/linux/clk-provider.h | 19 +++
> > 2 files changed, 217 insertions(+), 62 deletions(-)
>
> Sorry for the delay.
>
> With the fix you sent to Jeffrey
> Tested by porting the aoclk controller of Amlogic g12a SoC.
> This allowed to test
> * hws only table
> * parent_data with a mix of hw pointers and fw_name (with different input
> controllers and also an input that is optional and never provided)
>
> Tested-by: Jerome Brunet <jbrunet@baylibre.com>
>
> With the small comment below
>
> Reviewed-by Jerome Brunet <jbrunet@baylibre.com>
>
Awesome. Thanks! I need to Cc Rob H to hopefully get an ack on the
concept of relying on DT so I'll resend this series again next week. It
would also be nice if I can throw in a couple more patches to let
drivers specify a DT node when registering a clk if they don't have a
struct device on hand and let drivers lookup with clk_lookups somehow.
>
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 937b8d092d17..3d01e8c56400 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list);
> >
>
> [...]
>
> >
> > +static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
> > +{
> > + if (!src) {
> > + if (must_exist)
> > + return -EINVAL;
> > + return 0;
> > + }
> > +
> > + dst = kstrdup_const(src, GFP_KERNEL);
> > + if (!dst)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > +static int clk_core_populate_parent_map(struct clk_core *core)
> > +{
> > + const struct clk_init_data *init = core->hw->init;
> > + u8 num_parents = init->num_parents;
> > + const char * const *parent_names = init->parent_names;
> > + const struct clk_hw **parent_hws = init->parent_hws;
> > + const struct clk_parent_data *parent_data = init->parent_data;
> > + int i, ret = 0;
> > + struct clk_parent_map *parents, *parent;
> > +
> > + if (!num_parents)
> > + return 0;
> > +
> > + /*
> > + * Avoid unnecessary string look-ups of clk_core's possible parents by
> > + * having a cache of names/clk_hw pointers to clk_core pointers.
> > + */
> > + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> > + core->parents = parents;
> > + if (!parents)
> > + return -ENOMEM;
> > +
> > + /* Copy everything over because it might be __initdata */
> > + for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> > + if (parent_names) {
> > + /* throw a WARN if any entries are NULL */
> > + WARN(!parent_names[i],
> > + "%s: invalid NULL in %s's .parent_names\n",
> > + __func__, core->name);
> > + ret = clk_cpy_name(parent->name, parent_names[i],
> > + true);
> > + } else if (parent_data) {
>
> While testing, I mistakenly left both parent_names and parent_data. I was
> surprised that parent_data did not take precedence of parent_names.
>
> Maybe it should ? (... but I understand we are not supposed to provide both)
I don't think we can. We have a problem where drivers don't initialize
the init structure properly, opting to just throw it on the stack and
leave junk in there that they overwrite. We'd have to go through all the
init structures and initialize them. I suppose we could make a macro for
that:
DECLARE_CLK_INIT_DATA(init);
or something like that that does this. We could bury a magic number in
there under some debug option too so that we can make sure drivers are
doing this properly. Otherwise we're left to doing these weird tricks
like I've done here.
Regardless. I'll have to add a comment to this fact in the code. Thanks.
>
> > + parent->hw = parent_data[i].hw;
> > + ret = clk_cpy_name(parent->fw_name,
> > + parent_data[i].fw_name, false);
> > + if (!ret)
> > + ret = clk_cpy_name(parent->name,
> > + parent_data[i].name,
> > + false);
> > + } else if (parent_hws) {
> > + parent->hw = parent_hws[i];
> > + } else {
>
> Maybe there should also some kinda of check to verify if more than one option
> (among hws, parent_data and parent_names) was provided and throw a warn ?
>
> Could be useful with drivers move away from parent_names ?
Same thing. It would be nice but we're sort of unable to do so unless we
do what I suggest above. Should we do it?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names
2019-03-15 17:16 ` Stephen Boyd
@ 2019-03-19 9:25 ` Jerome Brunet
0 siblings, 0 replies; 18+ messages in thread
From: Jerome Brunet @ 2019-03-19 9:25 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette
Cc: linux-kernel, linux-clk, Miquel Raynal, Russell King,
Jeffrey Hugo, Chen-Yu Tsai
On Fri, 2019-03-15 at 10:16 -0700, Stephen Boyd wrote:
> Quoting Jerome Brunet (2019-03-15 03:01:53)
> > On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote:
> > > ---
> > > drivers/clk/clk.c | 260 ++++++++++++++++++++++++++---------
> > > include/linux/clk-provider.h | 19 +++
> > > 2 files changed, 217 insertions(+), 62 deletions(-)
> >
> > Sorry for the delay.
> >
> > With the fix you sent to Jeffrey
> > Tested by porting the aoclk controller of Amlogic g12a SoC.
> > This allowed to test
> > * hws only table
> > * parent_data with a mix of hw pointers and fw_name (with different input
> > controllers and also an input that is optional and never provided)
> >
> > Tested-by: Jerome Brunet <jbrunet@baylibre.com>
> >
> > With the small comment below
> >
> > Reviewed-by Jerome Brunet <jbrunet@baylibre.com>
> >
>
> Awesome. Thanks! I need to Cc Rob H to hopefully get an ack on the
> concept of relying on DT so I'll resend this series again next week. It
> would also be nice if I can throw in a couple more patches to let
> drivers specify a DT node when registering a clk if they don't have a
> struct device on hand and let drivers lookup with clk_lookups somehow.
>
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 937b8d092d17..3d01e8c56400 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list);
> > >
> >
> > [...]
> >
> > >
> > > +static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
> > > +{
> > > + if (!src) {
> > > + if (must_exist)
> > > + return -EINVAL;
> > > + return 0;
> > > + }
> > > +
> > > + dst = kstrdup_const(src, GFP_KERNEL);
> > > + if (!dst)
> > > + return -ENOMEM;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int clk_core_populate_parent_map(struct clk_core *core)
> > > +{
> > > + const struct clk_init_data *init = core->hw->init;
> > > + u8 num_parents = init->num_parents;
> > > + const char * const *parent_names = init->parent_names;
> > > + const struct clk_hw **parent_hws = init->parent_hws;
> > > + const struct clk_parent_data *parent_data = init->parent_data;
> > > + int i, ret = 0;
> > > + struct clk_parent_map *parents, *parent;
> > > +
> > > + if (!num_parents)
> > > + return 0;
> > > +
> > > + /*
> > > + * Avoid unnecessary string look-ups of clk_core's possible parents by
> > > + * having a cache of names/clk_hw pointers to clk_core pointers.
> > > + */
> > > + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> > > + core->parents = parents;
> > > + if (!parents)
> > > + return -ENOMEM;
> > > +
> > > + /* Copy everything over because it might be __initdata */
> > > + for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> > > + if (parent_names) {
> > > + /* throw a WARN if any entries are NULL */
> > > + WARN(!parent_names[i],
> > > + "%s: invalid NULL in %s's .parent_names\n",
> > > + __func__, core->name);
> > > + ret = clk_cpy_name(parent->name, parent_names[i],
> > > + true);
> > > + } else if (parent_data) {
> >
> > While testing, I mistakenly left both parent_names and parent_data. I was
> > surprised that parent_data did not take precedence of parent_names.
> >
> > Maybe it should ? (... but I understand we are not supposed to provide both)
>
> I don't think we can. We have a problem where drivers don't initialize
> the init structure properly, opting to just throw it on the stack and
> leave junk in there that they overwrite. We'd have to go through all the
> init structures and initialize them. I suppose we could make a macro for
> that:
>
> DECLARE_CLK_INIT_DATA(init);
>
> or something like that that does this. We could bury a magic number in
> there under some debug option too so that we can make sure drivers are
> doing this properly. Otherwise we're left to doing these weird tricks
> like I've done here.
>
> Regardless. I'll have to add a comment to this fact in the code. Thanks.
>
> > > + parent->hw = parent_data[i].hw;
> > > + ret = clk_cpy_name(parent->fw_name,
> > > + parent_data[i].fw_name, false);
> > > + if (!ret)
> > > + ret = clk_cpy_name(parent->name,
> > > + parent_data[i].name,
> > > + false);
> > > + } else if (parent_hws) {
> > > + parent->hw = parent_hws[i];
> > > + } else {
> >
> > Maybe there should also some kinda of check to verify if more than one option
> > (among hws, parent_data and parent_names) was provided and throw a warn ?
> >
> > Could be useful with drivers move away from parent_names ?
>
> Same thing. It would be nice but we're sort of unable to do so unless we
> do what I suggest above. Should we do it?
>
I was not thinking about anything complicated:
* Among the 3 pointers, just throw a warn if more than one is not NULL
* In the if/elseif/else, I would have put parent_data before parent_names
Nothing critical about that comment though
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-03-19 9:25 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-26 22:34 [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 1/8] clk: Combine __clk_get() and __clk_create_clk() Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 2/8] clk: core: clarify the check for runtime PM Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 3/8] clk: Introduce of_clk_get_hw_from_clkspec() Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 4/8] clk: Inform the core about consumer devices Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 5/8] clk: Move of_clk_*() APIs into clk.c from clkdev.c Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 6/8] clk: Allow parents to be specified without string names Stephen Boyd
2019-03-02 18:40 ` Jerome Brunet
2019-03-06 18:00 ` Stephen Boyd
2019-03-02 21:25 ` Jeffrey Hugo
2019-03-06 17:48 ` Stephen Boyd
2019-03-06 21:45 ` Jeffrey Hugo
2019-03-15 10:01 ` Jerome Brunet
2019-03-15 17:16 ` Stephen Boyd
2019-03-19 9:25 ` Jerome Brunet
2019-02-26 22:34 ` [PATCH v2 7/8] clk: qcom: gcc-sdm845: Migrate to DT parent mapping Stephen Boyd
2019-02-26 22:34 ` [PATCH v2 8/8] arm64: dts: qcom: Specify XO clk as input to GCC node Stephen Boyd
2019-03-02 0:45 ` [PATCH v2 0/8] Rewrite clk parent handling Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).