This patch series adds support in clock framework for clocks which operations
requires its parent clock is on.
Such clock type is initially met on Freescale i.MX7D platform that all clocks
operations, including enable/disable, rate change and re-parent, requires its
parent clock on. No sure if any other SoC has the similar clock type.
Current clock core can not support such type of clock well.
This patch introduce a new flag CLK_SET_PARENT_ON to handle this special case
in clock core that enable its parent clock firstly for each operation and disable
it later after operation complete.
The most special case is for set_parent() operation which requires both parent,
old one and new one, to be enabled at the same time during the operation.
Patch 1~3 are minor cleanup & fixes.
Patch 4 add CLK_SET_PARENT_ON flags in clock core to support such type clocks
Patch 5 show the need of introducing clk_core_enable_lock and
clk_core_disable_lock to easily use and reduce duplicated code.
It can be merged into patch 4 if required.
The patch series is based on for-next branch of Michael's git:
git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
Dong Aisheng (5):
clk: change clk_core name of __clk_set_parent_after
clk: add missing lock when call clk_core_enable in clk_set_parent
clk: remove unneeded __clk_enable and __clk_disable
clk: core: add CLK_SET_PARENT_ON flags to support clocks require
parent on
clk: introduce clk_core_enable_lock and clk_core_disable_lock
functions
drivers/clk/clk.c | 112 ++++++++++++++++++++++++++++++++++---------
include/linux/clk-provider.h | 5 ++
2 files changed, 94 insertions(+), 23 deletions(-)
--
1.9.1
To align with __clk_set_parent_before and some others functions,
change the host clk name to 'clk' instead of 'core'.
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/clk/clk.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 459ce9d..cc56ba2 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1487,7 +1487,7 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
return old_parent;
}
-static void __clk_set_parent_after(struct clk_core *core,
+static void __clk_set_parent_after(struct clk_core *clk,
struct clk_core *parent,
struct clk_core *old_parent)
{
@@ -1495,8 +1495,8 @@ static void __clk_set_parent_after(struct clk_core *core,
* Finish the migration of prepare state and undo the changes done
* for preventing a race with clk_enable().
*/
- if (core->prepare_count) {
- clk_core_disable(core);
+ if (clk->prepare_count) {
+ clk_core_disable(clk);
clk_core_disable(old_parent);
clk_core_unprepare(old_parent);
}
--
1.9.1
clk_core_enable is executed without &enable_clock in clk_set_parent function.
Adding it to avoid potential race condition issue.
Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances")
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/clk/clk.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cc56ba2..492644f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1475,8 +1475,10 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
*/
if (clk->prepare_count) {
clk_core_prepare(parent);
+ flags = clk_enable_lock();
clk_core_enable(parent);
clk_core_enable(clk);
+ clk_enable_unlock(flags);
}
/* update the clk tree topology */
@@ -1491,13 +1493,17 @@ static void __clk_set_parent_after(struct clk_core *clk,
struct clk_core *parent,
struct clk_core *old_parent)
{
+ unsigned long flags;
+
/*
* Finish the migration of prepare state and undo the changes done
* for preventing a race with clk_enable().
*/
if (clk->prepare_count) {
+ flags = clk_enable_lock();
clk_core_disable(clk);
clk_core_disable(old_parent);
+ clk_enable_unlock(flags);
clk_core_unprepare(old_parent);
}
}
@@ -1525,8 +1531,10 @@ static int __clk_set_parent(struct clk_core *clk, struct clk_core *parent,
clk_enable_unlock(flags);
if (clk->prepare_count) {
+ flags = clk_enable_lock();
clk_core_disable(clk);
clk_core_disable(parent);
+ clk_enable_unlock(flags);
clk_core_unprepare(parent);
}
return ret;
--
1.9.1
The only thing __clk_enable/__clk_disable does is NULL pointer checking
of clk except calling clk_core_{enable|disable} which is already handled
by clk_core_{enable|disable}.
So remove this unneeded function.
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/clk/clk.c | 20 ++------------------
1 file changed, 2 insertions(+), 18 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 492644f..7af553d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1023,14 +1023,6 @@ static void clk_core_disable(struct clk_core *clk)
clk_core_disable(clk->parent);
}
-static void __clk_disable(struct clk *clk)
-{
- if (!clk)
- return;
-
- clk_core_disable(clk->core);
-}
-
/**
* clk_disable - gate a clock
* @clk: the clk being gated
@@ -1051,7 +1043,7 @@ void clk_disable(struct clk *clk)
return;
flags = clk_enable_lock();
- __clk_disable(clk);
+ clk_core_disable(clk->core);
clk_enable_unlock(flags);
}
EXPORT_SYMBOL_GPL(clk_disable);
@@ -1089,14 +1081,6 @@ static int clk_core_enable(struct clk_core *clk)
return 0;
}
-static int __clk_enable(struct clk *clk)
-{
- if (!clk)
- return 0;
-
- return clk_core_enable(clk->core);
-}
-
/**
* clk_enable - ungate a clock
* @clk: the clk being ungated
@@ -1116,7 +1100,7 @@ int clk_enable(struct clk *clk)
int ret;
flags = clk_enable_lock();
- ret = __clk_enable(clk);
+ ret = clk_core_enable(clk->core);
clk_enable_unlock(flags);
return ret;
--
1.9.1
On Freescale i.MX7D platform, all clocks operations, including enable/disable,
rate change and re-parent, requires its parent clock on.
Current clock core can not support it well.
This patch introduce a new flag CLK_SET_PARENT_ON to handle this special case
in clock core that enable its parent clock firstly for each operation and disable
it later after operation complete.
The most special case is for set_parent() operation which requires both parent,
old one and new one, to be enabled at the same time during the operation.
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/clk/clk.c | 99 ++++++++++++++++++++++++++++++++++++++++----
include/linux/clk-provider.h | 5 +++
2 files changed, 95 insertions(+), 9 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7af553d..f2470e5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -43,6 +43,11 @@ static int clk_core_get_phase(struct clk_core *clk);
static bool clk_core_is_prepared(struct clk_core *clk);
static bool clk_core_is_enabled(struct clk_core *clk);
static struct clk_core *clk_core_lookup(const char *name);
+static struct clk *clk_core_get_parent(struct clk_core *clk);
+static int clk_core_prepare(struct clk_core *clk);
+static void clk_core_unprepare(struct clk_core *clk);
+static int clk_core_enable(struct clk_core *clk);
+static void clk_core_disable(struct clk_core *clk);
/*** private data structures ***/
@@ -508,6 +513,7 @@ static void clk_unprepare_unused_subtree(struct clk_core *clk)
static void clk_disable_unused_subtree(struct clk_core *clk)
{
struct clk_core *child;
+ struct clk *parent = clk_core_get_parent(clk);
unsigned long flags;
lockdep_assert_held(&prepare_lock);
@@ -515,6 +521,13 @@ static void clk_disable_unused_subtree(struct clk_core *clk)
hlist_for_each_entry(child, &clk->children, child_node)
clk_disable_unused_subtree(child);
+ if (clk->flags & CLK_SET_PARENT_ON && parent) {
+ clk_core_prepare(parent->core);
+ flags = clk_enable_lock();
+ clk_core_enable(parent->core);
+ clk_enable_unlock(flags);
+ }
+
flags = clk_enable_lock();
if (clk->enable_count)
@@ -539,6 +552,12 @@ static void clk_disable_unused_subtree(struct clk_core *clk)
unlock_out:
clk_enable_unlock(flags);
+ if (clk->flags & CLK_SET_PARENT_ON && parent) {
+ flags = clk_enable_lock();
+ clk_core_disable(parent->core);
+ clk_enable_unlock(flags);
+ clk_core_unprepare(parent->core);
+ }
}
static bool clk_ignore_unused;
@@ -608,6 +627,14 @@ struct clk *__clk_get_parent(struct clk *clk)
}
EXPORT_SYMBOL_GPL(__clk_get_parent);
+static struct clk *clk_core_get_parent(struct clk_core *clk)
+{
+ if (!clk)
+ return NULL;
+
+ return !clk->parent ? NULL : clk->parent->hw->clk;
+}
+
static struct clk_core *clk_core_get_parent_by_index(struct clk_core *clk,
u8 index)
{
@@ -1441,7 +1468,7 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
struct clk_core *old_parent = clk->parent;
/*
- * Migrate prepare state between parents and prevent race with
+ * 1. Migrate prepare state between parents and prevent race with
* clk_enable().
*
* If the clock is not prepared, then a race with
@@ -1456,13 +1483,27 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
* hardware and software states.
*
* See also: Comment for clk_set_parent() below.
+ *
+ * 2. enable two parents clock for .set_parent() operation if finding
+ * flag CLK_SET_PARENT_ON
*/
- if (clk->prepare_count) {
+ if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
clk_core_prepare(parent);
flags = clk_enable_lock();
clk_core_enable(parent);
- clk_core_enable(clk);
clk_enable_unlock(flags);
+
+ if (clk->prepare_count) {
+ flags = clk_enable_lock();
+ clk_core_enable(clk);
+ clk_enable_unlock(flags);
+ } else {
+
+ clk_core_prepare(old_parent);
+ flags = clk_enable_lock();
+ clk_core_enable(old_parent);
+ clk_enable_unlock(flags);
+ }
}
/* update the clk tree topology */
@@ -1483,12 +1524,22 @@ static void __clk_set_parent_after(struct clk_core *clk,
* Finish the migration of prepare state and undo the changes done
* for preventing a race with clk_enable().
*/
- if (clk->prepare_count) {
+ if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
flags = clk_enable_lock();
- clk_core_disable(clk);
clk_core_disable(old_parent);
clk_enable_unlock(flags);
clk_core_unprepare(old_parent);
+
+ if (clk->prepare_count) {
+ flags = clk_enable_lock();
+ clk_core_disable(clk);
+ clk_enable_unlock(flags);
+ } else {
+ flags = clk_enable_lock();
+ clk_core_disable(parent);
+ clk_enable_unlock(flags);
+ clk_core_unprepare(parent);
+ }
}
}
@@ -1514,12 +1565,23 @@ static int __clk_set_parent(struct clk_core *clk, struct clk_core *parent,
clk_reparent(clk, old_parent);
clk_enable_unlock(flags);
- if (clk->prepare_count) {
+ if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
flags = clk_enable_lock();
- clk_core_disable(clk);
clk_core_disable(parent);
clk_enable_unlock(flags);
clk_core_unprepare(parent);
+
+ if (clk->prepare_count) {
+ flags = clk_enable_lock();
+ clk_core_disable(clk);
+ clk_enable_unlock(flags);
+ } else {
+ flags = clk_enable_lock();
+ clk_core_disable(old_parent);
+ clk_enable_unlock(flags);
+ clk_core_unprepare(old_parent);
+ }
+
}
return ret;
}
@@ -1735,13 +1797,18 @@ static void clk_change_rate(struct clk_core *clk)
unsigned long best_parent_rate = 0;
bool skip_set_rate = false;
struct clk_core *old_parent;
+ struct clk_core *parent = NULL;
+ unsigned long flags;
old_rate = clk->rate;
- if (clk->new_parent)
+ if (clk->new_parent) {
+ parent = clk->new_parent;
best_parent_rate = clk->new_parent->rate;
- else if (clk->parent)
+ } else if (clk->parent) {
+ parent = clk->parent;
best_parent_rate = clk->parent->rate;
+ }
if (clk->new_parent && clk->new_parent != clk->parent) {
old_parent = __clk_set_parent_before(clk, clk->new_parent);
@@ -1762,6 +1829,13 @@ static void clk_change_rate(struct clk_core *clk)
trace_clk_set_rate(clk, clk->new_rate);
+ if (clk->flags & CLK_SET_PARENT_ON && parent) {
+ clk_core_prepare(parent);
+ flags = clk_enable_lock();
+ clk_core_enable(parent);
+ clk_enable_unlock(flags);
+ }
+
if (!skip_set_rate && clk->ops->set_rate)
clk->ops->set_rate(clk->hw, clk->new_rate, best_parent_rate);
@@ -1769,6 +1843,13 @@ static void clk_change_rate(struct clk_core *clk)
clk->rate = clk_recalc(clk, best_parent_rate);
+ if (clk->flags & CLK_SET_PARENT_ON && parent) {
+ flags = clk_enable_lock();
+ clk_core_disable(parent);
+ clk_enable_unlock(flags);
+ clk_core_unprepare(parent);
+ }
+
if (clk->notifier_count && old_rate != clk->rate)
__clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index df69531..242b966 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -31,6 +31,11 @@
#define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */
#define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
#define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
+/*
+ * parent clock must be on across any operation including
+ * clock gate/ungate, rate change and re-parent
+ */
+#define CLK_SET_PARENT_ON BIT(9)
struct clk_hw;
struct clk_core;
--
1.9.1
This can be usefully when clock core wants to enable/disable clocks.
Then we don't have to convert the struct clk_core to struct clk to call
clk_enable/clk_disable which is a bit un-align with exist using.
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
drivers/clk/clk.c | 79 +++++++++++++++++++++++++------------------------------
1 file changed, 36 insertions(+), 43 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f2470e5..6c481e7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -48,6 +48,8 @@ static int clk_core_prepare(struct clk_core *clk);
static void clk_core_unprepare(struct clk_core *clk);
static int clk_core_enable(struct clk_core *clk);
static void clk_core_disable(struct clk_core *clk);
+static int clk_core_enable_lock(struct clk_core *clk);
+static void clk_core_disable_lock(struct clk_core *clk);
/*** private data structures ***/
@@ -523,9 +525,7 @@ static void clk_disable_unused_subtree(struct clk_core *clk)
if (clk->flags & CLK_SET_PARENT_ON && parent) {
clk_core_prepare(parent->core);
- flags = clk_enable_lock();
- clk_core_enable(parent->core);
- clk_enable_unlock(flags);
+ clk_core_enable_lock(parent->core);
}
flags = clk_enable_lock();
@@ -553,9 +553,7 @@ static void clk_disable_unused_subtree(struct clk_core *clk)
unlock_out:
clk_enable_unlock(flags);
if (clk->flags & CLK_SET_PARENT_ON && parent) {
- flags = clk_enable_lock();
- clk_core_disable(parent->core);
- clk_enable_unlock(flags);
+ clk_core_disable_lock(parent->core);
clk_core_unprepare(parent->core);
}
}
@@ -1050,6 +1048,15 @@ static void clk_core_disable(struct clk_core *clk)
clk_core_disable(clk->parent);
}
+static void clk_core_disable_lock(struct clk_core *clk)
+{
+ unsigned long flags;
+
+ flags = clk_enable_lock();
+ clk_core_disable(clk);
+ clk_enable_unlock(flags);
+}
+
/**
* clk_disable - gate a clock
* @clk: the clk being gated
@@ -1108,6 +1115,18 @@ static int clk_core_enable(struct clk_core *clk)
return 0;
}
+static int clk_core_enable_lock(struct clk_core *clk)
+{
+ unsigned long flags;
+ int ret;
+
+ flags = clk_enable_lock();
+ ret = clk_core_enable(clk);
+ clk_enable_unlock(flags);
+
+ return ret;
+}
+
/**
* clk_enable - ungate a clock
* @clk: the clk being ungated
@@ -1489,20 +1508,13 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
*/
if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
clk_core_prepare(parent);
- flags = clk_enable_lock();
- clk_core_enable(parent);
- clk_enable_unlock(flags);
+ clk_core_enable_lock(parent);
if (clk->prepare_count) {
- flags = clk_enable_lock();
- clk_core_enable(clk);
- clk_enable_unlock(flags);
+ clk_core_enable_lock(clk);
} else {
-
clk_core_prepare(old_parent);
- flags = clk_enable_lock();
- clk_core_enable(old_parent);
- clk_enable_unlock(flags);
+ clk_core_enable_lock(old_parent);
}
}
@@ -1518,26 +1530,18 @@ static void __clk_set_parent_after(struct clk_core *clk,
struct clk_core *parent,
struct clk_core *old_parent)
{
- unsigned long flags;
-
/*
* Finish the migration of prepare state and undo the changes done
* for preventing a race with clk_enable().
*/
if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
- flags = clk_enable_lock();
- clk_core_disable(old_parent);
- clk_enable_unlock(flags);
+ clk_core_disable_lock(old_parent);
clk_core_unprepare(old_parent);
if (clk->prepare_count) {
- flags = clk_enable_lock();
- clk_core_disable(clk);
- clk_enable_unlock(flags);
+ clk_core_disable_lock(clk);
} else {
- flags = clk_enable_lock();
- clk_core_disable(parent);
- clk_enable_unlock(flags);
+ clk_core_disable_lock(parent);
clk_core_unprepare(parent);
}
}
@@ -1566,19 +1570,13 @@ static int __clk_set_parent(struct clk_core *clk, struct clk_core *parent,
clk_enable_unlock(flags);
if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
- flags = clk_enable_lock();
- clk_core_disable(parent);
- clk_enable_unlock(flags);
+ clk_core_disable_lock(parent);
clk_core_unprepare(parent);
if (clk->prepare_count) {
- flags = clk_enable_lock();
- clk_core_disable(clk);
- clk_enable_unlock(flags);
+ clk_core_disable_lock(clk);
} else {
- flags = clk_enable_lock();
- clk_core_disable(old_parent);
- clk_enable_unlock(flags);
+ clk_core_disable_lock(old_parent);
clk_core_unprepare(old_parent);
}
@@ -1798,7 +1796,6 @@ static void clk_change_rate(struct clk_core *clk)
bool skip_set_rate = false;
struct clk_core *old_parent;
struct clk_core *parent = NULL;
- unsigned long flags;
old_rate = clk->rate;
@@ -1831,9 +1828,7 @@ static void clk_change_rate(struct clk_core *clk)
if (clk->flags & CLK_SET_PARENT_ON && parent) {
clk_core_prepare(parent);
- flags = clk_enable_lock();
- clk_core_enable(parent);
- clk_enable_unlock(flags);
+ clk_core_enable_lock(parent);
}
if (!skip_set_rate && clk->ops->set_rate)
@@ -1844,9 +1839,7 @@ static void clk_change_rate(struct clk_core *clk)
clk->rate = clk_recalc(clk, best_parent_rate);
if (clk->flags & CLK_SET_PARENT_ON && parent) {
- flags = clk_enable_lock();
- clk_core_disable(parent);
- clk_enable_unlock(flags);
+ clk_core_disable_lock(parent);
clk_core_unprepare(parent);
}
--
1.9.1
On Wed, Apr 15, 2015 at 10:26:34PM +0800, Dong Aisheng wrote:
> This patch series adds support in clock framework for clocks which operations
> requires its parent clock is on.
>
> Such clock type is initially met on Freescale i.MX7D platform that all clocks
> operations, including enable/disable, rate change and re-parent, requires its
> parent clock on. No sure if any other SoC has the similar clock type.
>
> Current clock core can not support such type of clock well.
>
> This patch introduce a new flag CLK_SET_PARENT_ON to handle this special case
> in clock core that enable its parent clock firstly for each operation and disable
> it later after operation complete.
>
> The most special case is for set_parent() operation which requires both parent,
> old one and new one, to be enabled at the same time during the operation.
>
> Patch 1~3 are minor cleanup & fixes.
> Patch 4 add CLK_SET_PARENT_ON flags in clock core to support such type clocks
> Patch 5 show the need of introducing clk_core_enable_lock and
> clk_core_disable_lock to easily use and reduce duplicated code.
> It can be merged into patch 4 if required.
>
> The patch series is based on for-next branch of Michael's git:
> git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
>
> Dong Aisheng (5):
> clk: change clk_core name of __clk_set_parent_after
> clk: add missing lock when call clk_core_enable in clk_set_parent
> clk: remove unneeded __clk_enable and __clk_disable
> clk: core: add CLK_SET_PARENT_ON flags to support clocks require
> parent on
> clk: introduce clk_core_enable_lock and clk_core_disable_lock
> functions
>
Gentle Ping.
Regards
Dong Aisheng
> drivers/clk/clk.c | 112 ++++++++++++++++++++++++++++++++++---------
> include/linux/clk-provider.h | 5 ++
> 2 files changed, 94 insertions(+), 23 deletions(-)
>
> --
> 1.9.1
>
Hi Guys,
On Wed, Apr 22, 2015 at 02:12:43PM +0800, Dong Aisheng wrote:
> On Wed, Apr 15, 2015 at 10:26:34PM +0800, Dong Aisheng wrote:
> > This patch series adds support in clock framework for clocks which operations
> > requires its parent clock is on.
> >
> > Such clock type is initially met on Freescale i.MX7D platform that all clocks
> > operations, including enable/disable, rate change and re-parent, requires its
> > parent clock on. No sure if any other SoC has the similar clock type.
> >
> > Current clock core can not support such type of clock well.
> >
> > This patch introduce a new flag CLK_SET_PARENT_ON to handle this special case
> > in clock core that enable its parent clock firstly for each operation and disable
> > it later after operation complete.
> >
> > The most special case is for set_parent() operation which requires both parent,
> > old one and new one, to be enabled at the same time during the operation.
> >
> > Patch 1~3 are minor cleanup & fixes.
> > Patch 4 add CLK_SET_PARENT_ON flags in clock core to support such type clocks
> > Patch 5 show the need of introducing clk_core_enable_lock and
> > clk_core_disable_lock to easily use and reduce duplicated code.
> > It can be merged into patch 4 if required.
> >
> > The patch series is based on for-next branch of Michael's git:
> > git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
> >
> > Dong Aisheng (5):
> > clk: change clk_core name of __clk_set_parent_after
> > clk: add missing lock when call clk_core_enable in clk_set_parent
> > clk: remove unneeded __clk_enable and __clk_disable
> > clk: core: add CLK_SET_PARENT_ON flags to support clocks require
> > parent on
> > clk: introduce clk_core_enable_lock and clk_core_disable_lock
> > functions
> >
>
> Gentle Ping.
>
Ping again..
Any comments about this patch series?
Regards
Dong Aisheng
> Regards
> Dong Aisheng
>
> > drivers/clk/clk.c | 112 ++++++++++++++++++++++++++++++++++---------
> > include/linux/clk-provider.h | 5 ++
> > 2 files changed, 94 insertions(+), 23 deletions(-)
> >
> > --
> > 1.9.1
> >
On 04/15/15 07:26, Dong Aisheng wrote:
> To align with __clk_set_parent_before and some others functions,
> change the host clk name to 'clk' instead of 'core'.
>
> Cc: Mike Turquette <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
No thanks. We should go and rename all clk_core pointers in this file
from clk to core now that we're done splitting the two. I'll go do that now.
> drivers/clk/clk.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 459ce9d..cc56ba2 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1487,7 +1487,7 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
> return old_parent;
> }
>
> -static void __clk_set_parent_after(struct clk_core *core,
> +static void __clk_set_parent_after(struct clk_core *clk,
> struct clk_core *parent,
> struct clk_core *old_parent)
> {
> @@ -1495,8 +1495,8 @@ static void __clk_set_parent_after(struct clk_core *core,
> * Finish the migration of prepare state and undo the changes done
> * for preventing a race with clk_enable().
> */
> - if (core->prepare_count) {
> - clk_core_disable(core);
> + if (clk->prepare_count) {
> + clk_core_disable(clk);
> clk_core_disable(old_parent);
> clk_core_unprepare(old_parent);
> }
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/15/15 07:26, Dong Aisheng wrote:
> clk_core_enable is executed without &enable_clock in clk_set_parent function.
> Adding it to avoid potential race condition issue.
>
> Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances")
> Cc: Mike Turquette <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
Can you please describe the race condition? From what I can tell there
is not a race condition here and we've gone around on this part of the
code before to fix any race conditions.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/15/15 07:26, Dong Aisheng wrote:
> The only thing __clk_enable/__clk_disable does is NULL pointer checking
> of clk except calling clk_core_{enable|disable} which is already handled
> by clk_core_{enable|disable}.
> So remove this unneeded function.
>
> Cc: Mike Turquette <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
No. You can call clk_enable() and clk_disable() with NULL and it should
be a no-op. With this change it would cause a NULL pointer exception.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/30, Stephen Boyd wrote:
> On 04/15/15 07:26, Dong Aisheng wrote:
> > The only thing __clk_enable/__clk_disable does is NULL pointer checking
> > of clk except calling clk_core_{enable|disable} which is already handled
> > by clk_core_{enable|disable}.
> > So remove this unneeded function.
> >
> > Cc: Mike Turquette <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
>
> No. You can call clk_enable() and clk_disable() with NULL and it should
> be a no-op. With this change it would cause a NULL pointer exception.
>
Here's the proper patch. I'll leave you as author.
---8<---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0227ac3d5b1a..e45255226ffa 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1023,14 +1023,6 @@ static void clk_core_disable(struct clk_core *core)
clk_core_disable(core->parent);
}
-static void __clk_disable(struct clk *clk)
-{
- if (!clk)
- return;
-
- clk_core_disable(clk->core);
-}
-
/**
* clk_disable - gate a clock
* @clk: the clk being gated
@@ -1051,7 +1043,7 @@ void clk_disable(struct clk *clk)
return;
flags = clk_enable_lock();
- __clk_disable(clk);
+ clk_core_disable(clk->core);
clk_enable_unlock(flags);
}
EXPORT_SYMBOL_GPL(clk_disable);
@@ -1089,14 +1081,6 @@ static int clk_core_enable(struct clk_core *core)
return 0;
}
-static int __clk_enable(struct clk *clk)
-{
- if (!clk)
- return 0;
-
- return clk_core_enable(clk->core);
-}
-
/**
* clk_enable - ungate a clock
* @clk: the clk being ungated
@@ -1115,8 +1099,11 @@ int clk_enable(struct clk *clk)
unsigned long flags;
int ret;
+ if (!clk)
+ return 0;
+
flags = clk_enable_lock();
- ret = __clk_enable(clk);
+ ret = clk_core_enable(clk->core);
clk_enable_unlock(flags);
return ret;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/15, Dong Aisheng wrote:
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 7af553d..f2470e5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -43,6 +43,11 @@ static int clk_core_get_phase(struct clk_core *clk);
> static bool clk_core_is_prepared(struct clk_core *clk);
> static bool clk_core_is_enabled(struct clk_core *clk);
> static struct clk_core *clk_core_lookup(const char *name);
> +static struct clk *clk_core_get_parent(struct clk_core *clk);
> +static int clk_core_prepare(struct clk_core *clk);
> +static void clk_core_unprepare(struct clk_core *clk);
> +static int clk_core_enable(struct clk_core *clk);
> +static void clk_core_disable(struct clk_core *clk);
Let's avoid adding more here if we can.
>
> /*** private data structures ***/
>
> @@ -508,6 +513,7 @@ static void clk_unprepare_unused_subtree(struct clk_core *clk)
> static void clk_disable_unused_subtree(struct clk_core *clk)
> {
> struct clk_core *child;
> + struct clk *parent = clk_core_get_parent(clk);
> unsigned long flags;
>
> lockdep_assert_held(&prepare_lock);
> @@ -515,6 +521,13 @@ static void clk_disable_unused_subtree(struct clk_core *clk)
> hlist_for_each_entry(child, &clk->children, child_node)
> clk_disable_unused_subtree(child);
>
> + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> + clk_core_prepare(parent->core);
> + flags = clk_enable_lock();
> + clk_core_enable(parent->core);
> + clk_enable_unlock(flags);
> + }
If there's a parent and this clock is on, why wouldn't the parent
also be on? It doesn't seem right to have a clock that's on
without it's parent on that we're trying to turn off. Put another
way, how is this fixing anything?
> +
> flags = clk_enable_lock();
>
> if (clk->enable_count)
> @@ -608,6 +627,14 @@ struct clk *__clk_get_parent(struct clk *clk)
> }
> EXPORT_SYMBOL_GPL(__clk_get_parent);
>
> +static struct clk *clk_core_get_parent(struct clk_core *clk)
> +{
> + if (!clk)
> + return NULL;
> +
> + return !clk->parent ? NULL : clk->parent->hw->clk;
> +}
s/clk/core/ in this function
> +
> static struct clk_core *clk_core_get_parent_by_index(struct clk_core *clk,
> u8 index)
> {
> @@ -1456,13 +1483,27 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
> * hardware and software states.
> *
> * See also: Comment for clk_set_parent() below.
> + *
> + * 2. enable two parents clock for .set_parent() operation if finding
> + * flag CLK_SET_PARENT_ON
> */
> - if (clk->prepare_count) {
> + if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
> clk_core_prepare(parent);
> flags = clk_enable_lock();
> clk_core_enable(parent);
> - clk_core_enable(clk);
> clk_enable_unlock(flags);
> +
> + if (clk->prepare_count) {
> + flags = clk_enable_lock();
> + clk_core_enable(clk);
> + clk_enable_unlock(flags);
> + } else {
> +
> + clk_core_prepare(old_parent);
> + flags = clk_enable_lock();
> + clk_core_enable(old_parent);
> + clk_enable_unlock(flags);
> + }
> }
>
> /* update the clk tree topology */
> @@ -1483,12 +1524,22 @@ static void __clk_set_parent_after(struct clk_core *clk,
> * Finish the migration of prepare state and undo the changes done
> * for preventing a race with clk_enable().
> */
> - if (clk->prepare_count) {
> + if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
> flags = clk_enable_lock();
> - clk_core_disable(clk);
> clk_core_disable(old_parent);
> clk_enable_unlock(flags);
> clk_core_unprepare(old_parent);
> +
> + if (clk->prepare_count) {
> + flags = clk_enable_lock();
> + clk_core_disable(clk);
> + clk_enable_unlock(flags);
> + } else {
> + flags = clk_enable_lock();
> + clk_core_disable(parent);
> + clk_enable_unlock(flags);
> + clk_core_unprepare(parent);
> + }
Is there a reason why the clk itself can't be on when we switch
parents? It seems that if the clk was on during the parent
switch, then it should be possible to just add a flag check on
both these if conditions and be done. It may be possible to
change the behavior here and not enable the clk in hardware, just
up the count and turn on both the parents. I'm trying to recall
why we enable the clk itself across the switch.
> }
> }
>
> @@ -1514,12 +1565,23 @@ static int __clk_set_parent(struct clk_core *clk, struct clk_core *parent,
> clk_reparent(clk, old_parent);
> clk_enable_unlock(flags);
>
> - if (clk->prepare_count) {
> + if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
> flags = clk_enable_lock();
> - clk_core_disable(clk);
> clk_core_disable(parent);
> clk_enable_unlock(flags);
> clk_core_unprepare(parent);
> +
> + if (clk->prepare_count) {
> + flags = clk_enable_lock();
> + clk_core_disable(clk);
> + clk_enable_unlock(flags);
> + } else {
> + flags = clk_enable_lock();
> + clk_core_disable(old_parent);
> + clk_enable_unlock(flags);
> + clk_core_unprepare(old_parent);
> + }
> +
Hmmm... if __clk_set_parent_after() actually used the second
argument then we could put this duplicate logic in there and call
it with a different order of arguments in the success vs. error
paths in this function. Unless I missed something.
> }
> return ret;
> }
> @@ -1735,13 +1797,18 @@ static void clk_change_rate(struct clk_core *clk)
> unsigned long best_parent_rate = 0;
> bool skip_set_rate = false;
> struct clk_core *old_parent;
> + struct clk_core *parent = NULL;
> + unsigned long flags;
>
> old_rate = clk->rate;
>
> - if (clk->new_parent)
> + if (clk->new_parent) {
> + parent = clk->new_parent;
> best_parent_rate = clk->new_parent->rate;
> - else if (clk->parent)
> + } else if (clk->parent) {
> + parent = clk->parent;
> best_parent_rate = clk->parent->rate;
> + }
>
> if (clk->new_parent && clk->new_parent != clk->parent) {
> old_parent = __clk_set_parent_before(clk, clk->new_parent);
> @@ -1762,6 +1829,13 @@ static void clk_change_rate(struct clk_core *clk)
>
> trace_clk_set_rate(clk, clk->new_rate);
>
> + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> + clk_core_prepare(parent);
> + flags = clk_enable_lock();
> + clk_core_enable(parent);
> + clk_enable_unlock(flags);
> + }
I can understand the case where clk_set_parent() can't switch the
mux because it needs the source and destination parents to be
clocking. I have that hardware design on my desk. But to change
the rate of a clock? The name of the flag, CLK_SET_PARENT_ON,
leads me to believe we don't really need to do this if we're
changing the rate, unless we're also switching the parents. Care
to explain why the hardware requires this?
If we actually do need to keep the parent clock on when the rate
is switching the name of the flag could be better and not have
"set parent" in the name.
> +
> if (!skip_set_rate && clk->ops->set_rate)
> clk->ops->set_rate(clk->hw, clk->new_rate, best_parent_rate);
>
> @@ -1769,6 +1843,13 @@ static void clk_change_rate(struct clk_core *clk)
>
> clk->rate = clk_recalc(clk, best_parent_rate);
>
> + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> + flags = clk_enable_lock();
> + clk_core_disable(parent);
> + clk_enable_unlock(flags);
> + clk_core_unprepare(parent);
> + }
Why not just call clk_prepare_enable()? Or add a clk_core
specific function, clk_core_prepare_enable() that we can call
here. We could put the parent pointer check in there too so that
it's just
if (clk->flags & CLK_SET_PARENT_ON)
clk_core_prepare_enable(parent);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/15, Dong Aisheng wrote:
> This can be usefully when clock core wants to enable/disable clocks.
> Then we don't have to convert the struct clk_core to struct clk to call
> clk_enable/clk_disable which is a bit un-align with exist using.
>
> Cc: Mike Turquette <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
Yeah let's add this patch either before patch 4 or squash it into
patch 4. Also, avoid adding more function prototypes please.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, Apr 30, 2015 at 12:06:13PM -0700, Stephen Boyd wrote:
> On 04/15/15 07:26, Dong Aisheng wrote:
> > To align with __clk_set_parent_before and some others functions,
> > change the host clk name to 'clk' instead of 'core'.
> >
> > Cc: Mike Turquette <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
>
> No thanks. We should go and rename all clk_core pointers in this file
> from clk to core now that we're done splitting the two. I'll go do that now.
>
Understand.
Thanks for this information.
Regards
Dong Aisheng
> > drivers/clk/clk.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 459ce9d..cc56ba2 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1487,7 +1487,7 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
> > return old_parent;
> > }
> >
> > -static void __clk_set_parent_after(struct clk_core *core,
> > +static void __clk_set_parent_after(struct clk_core *clk,
> > struct clk_core *parent,
> > struct clk_core *old_parent)
> > {
> > @@ -1495,8 +1495,8 @@ static void __clk_set_parent_after(struct clk_core *core,
> > * Finish the migration of prepare state and undo the changes done
> > * for preventing a race with clk_enable().
> > */
> > - if (core->prepare_count) {
> > - clk_core_disable(core);
> > + if (clk->prepare_count) {
> > + clk_core_disable(clk);
> > clk_core_disable(old_parent);
> > clk_core_unprepare(old_parent);
> > }
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Thu, Apr 30, 2015 at 03:05:48PM -0700, Stephen Boyd wrote:
> On 04/30, Stephen Boyd wrote:
> > On 04/15/15 07:26, Dong Aisheng wrote:
> > > The only thing __clk_enable/__clk_disable does is NULL pointer checking
> > > of clk except calling clk_core_{enable|disable} which is already handled
> > > by clk_core_{enable|disable}.
> > > So remove this unneeded function.
> > >
> > > Cc: Mike Turquette <[email protected]>
> > > Cc: Stephen Boyd <[email protected]>
> > > Signed-off-by: Dong Aisheng <[email protected]>
> > > ---
> >
> > No. You can call clk_enable() and clk_disable() with NULL and it should
> > be a no-op. With this change it would cause a NULL pointer exception.
> >
>
> Here's the proper patch. I'll leave you as author.
>
Got your point.
Thanks for the change.
Regards
Dong Aisheng
> ---8<---
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0227ac3d5b1a..e45255226ffa 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1023,14 +1023,6 @@ static void clk_core_disable(struct clk_core *core)
> clk_core_disable(core->parent);
> }
>
> -static void __clk_disable(struct clk *clk)
> -{
> - if (!clk)
> - return;
> -
> - clk_core_disable(clk->core);
> -}
> -
> /**
> * clk_disable - gate a clock
> * @clk: the clk being gated
> @@ -1051,7 +1043,7 @@ void clk_disable(struct clk *clk)
> return;
>
> flags = clk_enable_lock();
> - __clk_disable(clk);
> + clk_core_disable(clk->core);
> clk_enable_unlock(flags);
> }
> EXPORT_SYMBOL_GPL(clk_disable);
> @@ -1089,14 +1081,6 @@ static int clk_core_enable(struct clk_core *core)
> return 0;
> }
>
> -static int __clk_enable(struct clk *clk)
> -{
> - if (!clk)
> - return 0;
> -
> - return clk_core_enable(clk->core);
> -}
> -
> /**
> * clk_enable - ungate a clock
> * @clk: the clk being ungated
> @@ -1115,8 +1099,11 @@ int clk_enable(struct clk *clk)
> unsigned long flags;
> int ret;
>
> + if (!clk)
> + return 0;
> +
> flags = clk_enable_lock();
> - ret = __clk_enable(clk);
> + ret = clk_core_enable(clk->core);
> clk_enable_unlock(flags);
>
> return ret;
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
On Thu, Apr 30, 2015 at 12:07:47PM -0700, Stephen Boyd wrote:
> On 04/15/15 07:26, Dong Aisheng wrote:
> > clk_core_enable is executed without &enable_clock in clk_set_parent function.
> > Adding it to avoid potential race condition issue.
> >
> > Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances")
> > Cc: Mike Turquette <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
>
> Can you please describe the race condition? From what I can tell there
> is not a race condition here and we've gone around on this part of the
> code before to fix any race conditions.
>
Do you mean we do not need to acquire enable lock when execute clk_core_enable
in set_parent function? Can you help explain a bit more why?
The clk doc looks to me says the enable lock should be held across calls to
the .enable, .disable and .is_enabled operations.
And before the commit
035a61c314eb ("clk: Make clk API return per-user struct clk instances"),
all the clk_enable/disable in set_parent() is executed with lock.
A rough thinking of race condition is assuming Thread A calls
clk_set_parent(x, y) while Thread B calls clk_enable(x), clock x is disabled
but prepared initially, due to clk_core_enable in set_parent() is not
executed with enable clock, the clk_core_enable may be reentrant during
the locking time executed by B.
Won't this be a race condition?
Regards
Dong Aisheng
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Thu, Apr 30, 2015 at 06:09:38PM -0700, Stephen Boyd wrote:
> On 04/15, Dong Aisheng wrote:
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 7af553d..f2470e5 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -43,6 +43,11 @@ static int clk_core_get_phase(struct clk_core *clk);
> > static bool clk_core_is_prepared(struct clk_core *clk);
> > static bool clk_core_is_enabled(struct clk_core *clk);
> > static struct clk_core *clk_core_lookup(const char *name);
> > +static struct clk *clk_core_get_parent(struct clk_core *clk);
> > +static int clk_core_prepare(struct clk_core *clk);
> > +static void clk_core_unprepare(struct clk_core *clk);
> > +static int clk_core_enable(struct clk_core *clk);
> > +static void clk_core_disable(struct clk_core *clk);
>
> Let's avoid adding more here if we can.
>
Yes, also don't want to add these pre-declarations, but needed by the following
changes...
> >
> > /*** private data structures ***/
> >
> > @@ -508,6 +513,7 @@ static void clk_unprepare_unused_subtree(struct clk_core *clk)
> > static void clk_disable_unused_subtree(struct clk_core *clk)
> > {
> > struct clk_core *child;
> > + struct clk *parent = clk_core_get_parent(clk);
> > unsigned long flags;
> >
> > lockdep_assert_held(&prepare_lock);
> > @@ -515,6 +521,13 @@ static void clk_disable_unused_subtree(struct clk_core *clk)
> > hlist_for_each_entry(child, &clk->children, child_node)
> > clk_disable_unused_subtree(child);
> >
> > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > + clk_core_prepare(parent->core);
> > + flags = clk_enable_lock();
> > + clk_core_enable(parent->core);
> > + clk_enable_unlock(flags);
> > + }
>
> If there's a parent and this clock is on, why wouldn't the parent
> also be on? It doesn't seem right to have a clock that's on
> without it's parent on that we're trying to turn off. Put another
> way, how is this fixing anything?
>
Well, this is caused by a state mis-align between HW and SW in clock tree
during booting.
Usually in uboot, we may enable all clocks in HW by default.
And during kernel booting time, the parent clock could be disabled in its driver
probe due to calling clk_prepare_enable and clk_disable_unprepare.
Because it's child clock is only enabled in HW while its SW usecount in
clock tree is still 0, so clk_disable of parent clock will gate the parent clock
in both HW and SW usecount ultimately.
Then there will be a clock is on in HW while its parent is disabled.
Later when clock core does clk_disable_unused, this clock disable
will cause system hang due to the limitation of operation requiring its parent
clock on.
> > +
> > flags = clk_enable_lock();
> >
> > if (clk->enable_count)
> > @@ -608,6 +627,14 @@ struct clk *__clk_get_parent(struct clk *clk)
> > }
> > EXPORT_SYMBOL_GPL(__clk_get_parent);
> >
> > +static struct clk *clk_core_get_parent(struct clk_core *clk)
> > +{
> > + if (!clk)
> > + return NULL;
> > +
> > + return !clk->parent ? NULL : clk->parent->hw->clk;
> > +}
>
> s/clk/core/ in this function
>
Got it as you told we will change all clk name to core for clk_core.
> > +
> > static struct clk_core *clk_core_get_parent_by_index(struct clk_core *clk,
> > u8 index)
> > {
> > @@ -1456,13 +1483,27 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
> > * hardware and software states.
> > *
> > * See also: Comment for clk_set_parent() below.
> > + *
> > + * 2. enable two parents clock for .set_parent() operation if finding
> > + * flag CLK_SET_PARENT_ON
> > */
> > - if (clk->prepare_count) {
> > + if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
> > clk_core_prepare(parent);
> > flags = clk_enable_lock();
> > clk_core_enable(parent);
> > - clk_core_enable(clk);
> > clk_enable_unlock(flags);
> > +
> > + if (clk->prepare_count) {
> > + flags = clk_enable_lock();
> > + clk_core_enable(clk);
> > + clk_enable_unlock(flags);
> > + } else {
> > +
> > + clk_core_prepare(old_parent);
> > + flags = clk_enable_lock();
> > + clk_core_enable(old_parent);
> > + clk_enable_unlock(flags);
> > + }
> > }
> >
> > /* update the clk tree topology */
> > @@ -1483,12 +1524,22 @@ static void __clk_set_parent_after(struct clk_core *clk,
> > * Finish the migration of prepare state and undo the changes done
> > * for preventing a race with clk_enable().
> > */
> > - if (clk->prepare_count) {
> > + if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
> > flags = clk_enable_lock();
> > - clk_core_disable(clk);
> > clk_core_disable(old_parent);
> > clk_enable_unlock(flags);
> > clk_core_unprepare(old_parent);
> > +
> > + if (clk->prepare_count) {
> > + flags = clk_enable_lock();
> > + clk_core_disable(clk);
> > + clk_enable_unlock(flags);
> > + } else {
> > + flags = clk_enable_lock();
> > + clk_core_disable(parent);
> > + clk_enable_unlock(flags);
> > + clk_core_unprepare(parent);
> > + }
>
> Is there a reason why the clk itself can't be on when we switch
> parents?
The CLK_SET_PARENT_ON only indicates that it's parent clocks should
be on, not itself clock.
And we don't want to break CLK_SET_PARENT_GATE clocks.
So if the clk is not prepared before, we only enable its old parent,
not including itself.
> It seems that if the clk was on during the parent
> switch, then it should be possible to just add a flag check on
> both these if conditions and be done. It may be possible to
> change the behavior here and not enable the clk in hardware, just
> up the count and turn on both the parents. I'm trying to recall
> why we enable the clk itself across the switch.
>
Seems it's needed by this fix:
commit f8aa0b clk: Fix race condition between clk_set_parent and clk_enable()
> > }
> > }
> >
> > @@ -1514,12 +1565,23 @@ static int __clk_set_parent(struct clk_core *clk, struct clk_core *parent,
> > clk_reparent(clk, old_parent);
> > clk_enable_unlock(flags);
> >
> > - if (clk->prepare_count) {
> > + if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
> > flags = clk_enable_lock();
> > - clk_core_disable(clk);
> > clk_core_disable(parent);
> > clk_enable_unlock(flags);
> > clk_core_unprepare(parent);
> > +
> > + if (clk->prepare_count) {
> > + flags = clk_enable_lock();
> > + clk_core_disable(clk);
> > + clk_enable_unlock(flags);
> > + } else {
> > + flags = clk_enable_lock();
> > + clk_core_disable(old_parent);
> > + clk_enable_unlock(flags);
> > + clk_core_unprepare(old_parent);
> > + }
> > +
>
> Hmmm... if __clk_set_parent_after() actually used the second
> argument then we could put this duplicate logic in there and call
> it with a different order of arguments in the success vs. error
> paths in this function. Unless I missed something.
>
Good suggestion, will try in next version.
> > }
> > return ret;
> > }
> > @@ -1735,13 +1797,18 @@ static void clk_change_rate(struct clk_core *clk)
> > unsigned long best_parent_rate = 0;
> > bool skip_set_rate = false;
> > struct clk_core *old_parent;
> > + struct clk_core *parent = NULL;
> > + unsigned long flags;
> >
> > old_rate = clk->rate;
> >
> > - if (clk->new_parent)
> > + if (clk->new_parent) {
> > + parent = clk->new_parent;
> > best_parent_rate = clk->new_parent->rate;
> > - else if (clk->parent)
> > + } else if (clk->parent) {
> > + parent = clk->parent;
> > best_parent_rate = clk->parent->rate;
> > + }
> >
> > if (clk->new_parent && clk->new_parent != clk->parent) {
> > old_parent = __clk_set_parent_before(clk, clk->new_parent);
> > @@ -1762,6 +1829,13 @@ static void clk_change_rate(struct clk_core *clk)
> >
> > trace_clk_set_rate(clk, clk->new_rate);
> >
> > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > + clk_core_prepare(parent);
> > + flags = clk_enable_lock();
> > + clk_core_enable(parent);
> > + clk_enable_unlock(flags);
> > + }
>
> I can understand the case where clk_set_parent() can't switch the
> mux because it needs the source and destination parents to be
> clocking. I have that hardware design on my desk. But to change
> the rate of a clock? The name of the flag, CLK_SET_PARENT_ON,
> leads me to believe we don't really need to do this if we're
> changing the rate, unless we're also switching the parents. Care
> to explain why the hardware requires this?
>
What i know is that it's designed for eliminate glitch internally.
Will find more information for you.
> If we actually do need to keep the parent clock on when the rate
> is switching the name of the flag could be better and not have
> "set parent" in the name.
>
Actually it's meaning is the clock set(e.g. rate, gate/ungate)
needs its parent clocks on, not set parent...
But Yes, i fully agree the naming is a bit misleading.
It just follows the exist CLK_SET_X convention..
Seems not easy find a better name.
What's your suggestion?
How about CLK_OPS_PARENT_ON?
> > +
> > if (!skip_set_rate && clk->ops->set_rate)
> > clk->ops->set_rate(clk->hw, clk->new_rate, best_parent_rate);
> >
> > @@ -1769,6 +1843,13 @@ static void clk_change_rate(struct clk_core *clk)
> >
> > clk->rate = clk_recalc(clk, best_parent_rate);
> >
> > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > + flags = clk_enable_lock();
> > + clk_core_disable(parent);
> > + clk_enable_unlock(flags);
> > + clk_core_unprepare(parent);
> > + }
>
> Why not just call clk_prepare_enable()?
Caused by two reasons initially:
1) no one uses clk_prepare_enable in clk core, so i guess it may be better
not use it :-)
2) clk_prepare_enable includes one more unneeded prepare lock acquire,
lines exchange efficiency.
> Or add a clk_core
> specific function, clk_core_prepare_enable() that we can call
> here. We could put the parent pointer check in there too so that
> it's just
>
> if (clk->flags & CLK_SET_PARENT_ON)
> clk_core_prepare_enable(parent);
I'm ok if you like this. :)
Regards
Dong Aisheng
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
On Thu, Apr 30, 2015 at 06:10:43PM -0700, Stephen Boyd wrote:
> On 04/15, Dong Aisheng wrote:
> > This can be usefully when clock core wants to enable/disable clocks.
> > Then we don't have to convert the struct clk_core to struct clk to call
> > clk_enable/clk_disable which is a bit un-align with exist using.
> >
> > Cc: Mike Turquette <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
>
> Yeah let's add this patch either before patch 4 or squash it into
> patch 4. Also, avoid adding more function prototypes please.
>
Will do it.
Thanks for the suggestion.
Regards
Dong Aisheng
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
On 05/04, Dong Aisheng wrote:
> On Thu, Apr 30, 2015 at 06:09:38PM -0700, Stephen Boyd wrote:
> > On 04/15, Dong Aisheng wrote:
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 7af553d..f2470e5 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -43,6 +43,11 @@ static int clk_core_get_phase(struct clk_core *clk);
> > > static bool clk_core_is_prepared(struct clk_core *clk);
> > > static bool clk_core_is_enabled(struct clk_core *clk);
> > > static struct clk_core *clk_core_lookup(const char *name);
> > > +static struct clk *clk_core_get_parent(struct clk_core *clk);
> > > +static int clk_core_prepare(struct clk_core *clk);
> > > +static void clk_core_unprepare(struct clk_core *clk);
> > > +static int clk_core_enable(struct clk_core *clk);
> > > +static void clk_core_disable(struct clk_core *clk);
> >
> > Let's avoid adding more here if we can.
> >
>
> Yes, also don't want to add these pre-declarations, but needed by the following
> changes...
>
> > >
> > > /*** private data structures ***/
> > >
> > > @@ -508,6 +513,7 @@ static void clk_unprepare_unused_subtree(struct clk_core *clk)
> > > static void clk_disable_unused_subtree(struct clk_core *clk)
> > > {
> > > struct clk_core *child;
> > > + struct clk *parent = clk_core_get_parent(clk);
> > > unsigned long flags;
> > >
> > > lockdep_assert_held(&prepare_lock);
> > > @@ -515,6 +521,13 @@ static void clk_disable_unused_subtree(struct clk_core *clk)
> > > hlist_for_each_entry(child, &clk->children, child_node)
> > > clk_disable_unused_subtree(child);
> > >
> > > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > > + clk_core_prepare(parent->core);
> > > + flags = clk_enable_lock();
> > > + clk_core_enable(parent->core);
> > > + clk_enable_unlock(flags);
> > > + }
> >
> > If there's a parent and this clock is on, why wouldn't the parent
> > also be on? It doesn't seem right to have a clock that's on
> > without it's parent on that we're trying to turn off. Put another
> > way, how is this fixing anything?
> >
>
> Well, this is caused by a state mis-align between HW and SW in clock tree
> during booting.
> Usually in uboot, we may enable all clocks in HW by default.
> And during kernel booting time, the parent clock could be disabled in its driver
> probe due to calling clk_prepare_enable and clk_disable_unprepare.
> Because it's child clock is only enabled in HW while its SW usecount in
> clock tree is still 0, so clk_disable of parent clock will gate the parent clock
> in both HW and SW usecount ultimately.
> Then there will be a clock is on in HW while its parent is disabled.
>
> Later when clock core does clk_disable_unused, this clock disable
> will cause system hang due to the limitation of operation requiring its parent
> clock on.
>
Very good. Now we understand the real problem; the framework
doesn't keep parent clocks on for child clocks that are already
on in the bootloader when those child clocks haven't been
acquired by a driver. How about we solve that problem?
> > > @@ -1483,12 +1524,22 @@ static void __clk_set_parent_after(struct clk_core *clk,
> > > * Finish the migration of prepare state and undo the changes done
> > > * for preventing a race with clk_enable().
> > > */
> > > - if (clk->prepare_count) {
> > > + if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
> > > flags = clk_enable_lock();
> > > - clk_core_disable(clk);
> > > clk_core_disable(old_parent);
> > > clk_enable_unlock(flags);
> > > clk_core_unprepare(old_parent);
> > > +
> > > + if (clk->prepare_count) {
> > > + flags = clk_enable_lock();
> > > + clk_core_disable(clk);
> > > + clk_enable_unlock(flags);
> > > + } else {
> > > + flags = clk_enable_lock();
> > > + clk_core_disable(parent);
> > > + clk_enable_unlock(flags);
> > > + clk_core_unprepare(parent);
> > > + }
> >
> > Is there a reason why the clk itself can't be on when we switch
> > parents?
>
> The CLK_SET_PARENT_ON only indicates that it's parent clocks should
> be on, not itself clock.
> And we don't want to break CLK_SET_PARENT_GATE clocks.
> So if the clk is not prepared before, we only enable its old parent,
> not including itself.
>
> > It seems that if the clk was on during the parent
> > switch, then it should be possible to just add a flag check on
> > both these if conditions and be done. It may be possible to
> > change the behavior here and not enable the clk in hardware, just
> > up the count and turn on both the parents. I'm trying to recall
> > why we enable the clk itself across the switch.
> >
>
> Seems it's needed by this fix:
> commit f8aa0b clk: Fix race condition between clk_set_parent and clk_enable()
I'm saying I don't recall why that commit needed to turn on the
clock itself. It certainly doesn't seem to be required at first
glance. I hope we can get away with just upping the count on the
clock and enable both parents to avoid the race condition so that
we don't have to worry about CLK_SET_PARENT_GATE.
>
> > > }
> > > return ret;
> > > }
> > > @@ -1735,13 +1797,18 @@ static void clk_change_rate(struct clk_core *clk)
> > > unsigned long best_parent_rate = 0;
> > > bool skip_set_rate = false;
> > > struct clk_core *old_parent;
> > > + struct clk_core *parent = NULL;
> > > + unsigned long flags;
> > >
> > > old_rate = clk->rate;
> > >
> > > - if (clk->new_parent)
> > > + if (clk->new_parent) {
> > > + parent = clk->new_parent;
> > > best_parent_rate = clk->new_parent->rate;
> > > - else if (clk->parent)
> > > + } else if (clk->parent) {
> > > + parent = clk->parent;
> > > best_parent_rate = clk->parent->rate;
> > > + }
> > >
> > > if (clk->new_parent && clk->new_parent != clk->parent) {
> > > old_parent = __clk_set_parent_before(clk, clk->new_parent);
> > > @@ -1762,6 +1829,13 @@ static void clk_change_rate(struct clk_core *clk)
> > >
> > > trace_clk_set_rate(clk, clk->new_rate);
> > >
> > > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > > + clk_core_prepare(parent);
> > > + flags = clk_enable_lock();
> > > + clk_core_enable(parent);
> > > + clk_enable_unlock(flags);
> > > + }
> >
> > I can understand the case where clk_set_parent() can't switch the
> > mux because it needs the source and destination parents to be
> > clocking. I have that hardware design on my desk. But to change
> > the rate of a clock? The name of the flag, CLK_SET_PARENT_ON,
> > leads me to believe we don't really need to do this if we're
> > changing the rate, unless we're also switching the parents. Care
> > to explain why the hardware requires this?
> >
>
> What i know is that it's designed for eliminate glitch internally.
> Will find more information for you.
On my hardware we need to have both parents on to switch the mux
in a glitch free manner. Using the .set_rate_and_parent op we can
change the mux value and the rate (i.e. divider) at the same time
with one register write. And we only need to worry about having
both parents on if the clock itself is on, which the framework
handles for us today.
>
> > If we actually do need to keep the parent clock on when the rate
> > is switching the name of the flag could be better and not have
> > "set parent" in the name.
> >
>
> Actually it's meaning is the clock set(e.g. rate, gate/ungate)
> needs its parent clocks on, not set parent...
> But Yes, i fully agree the naming is a bit misleading.
>
> It just follows the exist CLK_SET_X convention..
> Seems not easy find a better name.
> What's your suggestion?
>
> How about CLK_OPS_PARENT_ON?
Sure.
>
> > > +
> > > if (!skip_set_rate && clk->ops->set_rate)
> > > clk->ops->set_rate(clk->hw, clk->new_rate, best_parent_rate);
> > >
> > > @@ -1769,6 +1843,13 @@ static void clk_change_rate(struct clk_core *clk)
> > >
> > > clk->rate = clk_recalc(clk, best_parent_rate);
> > >
> > > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > > + flags = clk_enable_lock();
> > > + clk_core_disable(parent);
> > > + clk_enable_unlock(flags);
> > > + clk_core_unprepare(parent);
> > > + }
> >
> > Why not just call clk_prepare_enable()?
>
> Caused by two reasons initially:
> 1) no one uses clk_prepare_enable in clk core, so i guess it may be better
> not use it :-)
> 2) clk_prepare_enable includes one more unneeded prepare lock acquire,
> lines exchange efficiency.
>
> > Or add a clk_core
> > specific function, clk_core_prepare_enable() that we can call
> > here. We could put the parent pointer check in there too so that
> > it's just
> >
> > if (clk->flags & CLK_SET_PARENT_ON)
> > clk_core_prepare_enable(parent);
>
> I'm ok if you like this. :)
>
Yes the helper is better than calling clk_prepare_enable()
directly.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 05/04, Dong Aisheng wrote:
> On Thu, Apr 30, 2015 at 12:07:47PM -0700, Stephen Boyd wrote:
> > On 04/15/15 07:26, Dong Aisheng wrote:
> > > clk_core_enable is executed without &enable_clock in clk_set_parent function.
> > > Adding it to avoid potential race condition issue.
> > >
> > > Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances")
> > > Cc: Mike Turquette <[email protected]>
> > > Cc: Stephen Boyd <[email protected]>
> > > Signed-off-by: Dong Aisheng <[email protected]>
> > > ---
> >
> > Can you please describe the race condition? From what I can tell there
> > is not a race condition here and we've gone around on this part of the
> > code before to fix any race conditions.
> >
>
> Do you mean we do not need to acquire enable lock when execute clk_core_enable
> in set_parent function? Can you help explain a bit more why?
>
> The clk doc looks to me says the enable lock should be held across calls to
> the .enable, .disable and .is_enabled operations.
>
> And before the commit
> 035a61c314eb ("clk: Make clk API return per-user struct clk instances"),
> all the clk_enable/disable in set_parent() is executed with lock.
>
> A rough thinking of race condition is assuming Thread A calls
> clk_set_parent(x, y) while Thread B calls clk_enable(x), clock x is disabled
> but prepared initially, due to clk_core_enable in set_parent() is not
> executed with enable clock, the clk_core_enable may be reentrant during
> the locking time executed by B.
> Won't this be a race condition?
>
Ah I see now. The commit text could say something like this:
Before commit 035a61c314eb ("clk: Make clk API return per-user
struct clk instances") we acquired the enable_lock in
__clk_set_parent_{before,after}() by means of calling
clk_enable(). After commit 035a61c314eb we use clk_core_enable()
in place of the clk_enable(), and clk_core_enable() doesn't
acquire the enable_lock. This opens up a race condition between
clk_set_parent() and clk_enable().
I've replaced the commit text and applied it to clk-fixes.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed, May 06, 2015 at 04:34:47PM -0700, Stephen Boyd wrote:
> On 05/04, Dong Aisheng wrote:
> > On Thu, Apr 30, 2015 at 06:09:38PM -0700, Stephen Boyd wrote:
> > > On 04/15, Dong Aisheng wrote:
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index 7af553d..f2470e5 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -43,6 +43,11 @@ static int clk_core_get_phase(struct clk_core *clk);
> > > > static bool clk_core_is_prepared(struct clk_core *clk);
> > > > static bool clk_core_is_enabled(struct clk_core *clk);
> > > > static struct clk_core *clk_core_lookup(const char *name);
> > > > +static struct clk *clk_core_get_parent(struct clk_core *clk);
> > > > +static int clk_core_prepare(struct clk_core *clk);
> > > > +static void clk_core_unprepare(struct clk_core *clk);
> > > > +static int clk_core_enable(struct clk_core *clk);
> > > > +static void clk_core_disable(struct clk_core *clk);
> > >
> > > Let's avoid adding more here if we can.
> > >
> >
> > Yes, also don't want to add these pre-declarations, but needed by the following
> > changes...
> >
> > > >
> > > > /*** private data structures ***/
> > > >
> > > > @@ -508,6 +513,7 @@ static void clk_unprepare_unused_subtree(struct clk_core *clk)
> > > > static void clk_disable_unused_subtree(struct clk_core *clk)
> > > > {
> > > > struct clk_core *child;
> > > > + struct clk *parent = clk_core_get_parent(clk);
> > > > unsigned long flags;
> > > >
> > > > lockdep_assert_held(&prepare_lock);
> > > > @@ -515,6 +521,13 @@ static void clk_disable_unused_subtree(struct clk_core *clk)
> > > > hlist_for_each_entry(child, &clk->children, child_node)
> > > > clk_disable_unused_subtree(child);
> > > >
> > > > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > > > + clk_core_prepare(parent->core);
> > > > + flags = clk_enable_lock();
> > > > + clk_core_enable(parent->core);
> > > > + clk_enable_unlock(flags);
> > > > + }
> > >
> > > If there's a parent and this clock is on, why wouldn't the parent
> > > also be on? It doesn't seem right to have a clock that's on
> > > without it's parent on that we're trying to turn off. Put another
> > > way, how is this fixing anything?
> > >
> >
> > Well, this is caused by a state mis-align between HW and SW in clock tree
> > during booting.
> > Usually in uboot, we may enable all clocks in HW by default.
> > And during kernel booting time, the parent clock could be disabled in its driver
> > probe due to calling clk_prepare_enable and clk_disable_unprepare.
> > Because it's child clock is only enabled in HW while its SW usecount in
> > clock tree is still 0, so clk_disable of parent clock will gate the parent clock
> > in both HW and SW usecount ultimately.
> > Then there will be a clock is on in HW while its parent is disabled.
> >
> > Later when clock core does clk_disable_unused, this clock disable
> > will cause system hang due to the limitation of operation requiring its parent
> > clock on.
> >
>
> Very good. Now we understand the real problem; the framework
> doesn't keep parent clocks on for child clocks that are already
> on in the bootloader when those child clocks haven't been
> acquired by a driver. How about we solve that problem?
>
Yes, actually we have already tried this way before to fix the issue
that don't gate parent clock in HW if any child is on during booting phase.
e.g. Clock A and B, B is child of A.
Both A and B clock are enabled in HW in bootloader.
Driver A probe:
clk_prepare_enable(A) -> clk_disable_unprepare(A)
In disable operation, clock framework finds there's a child B is on,
so framework will not gate clock A by calling ops->disable(), but instead,
only de-reference the enable_count, although it's enable_count becomes 0.
Then both A and B are still kept on in HW but not in clock tree state.
Later the clk_disable_unused feature in clock framework will gate those unused
clocks, both A and B, both HW and SW clock tree state are consistent.
We've tested this approach can work, but comparing to the approach we
finally used in this patch series, it has a few disadvantages:
1) The definition of the API clk_disable and clk_unprepare becomes a bit
slightly different from current one, it needs take count of the child clock
state while currently not.
2) It's more complicated and require more code changes.
3) more power consumption although not big deal
4) Manually create inconsistent state between HW and clock tree during
booting may be not good choice.
So we intend to the approach in this patch series that enable parent clock
during clk_disable_unused and disable it again if the clock type requires.
It's simple and easy understand.
> > > > @@ -1483,12 +1524,22 @@ static void __clk_set_parent_after(struct clk_core *clk,
> > > > * Finish the migration of prepare state and undo the changes done
> > > > * for preventing a race with clk_enable().
> > > > */
> > > > - if (clk->prepare_count) {
> > > > + if (clk->prepare_count || clk->flags & CLK_SET_PARENT_ON) {
> > > > flags = clk_enable_lock();
> > > > - clk_core_disable(clk);
> > > > clk_core_disable(old_parent);
> > > > clk_enable_unlock(flags);
> > > > clk_core_unprepare(old_parent);
> > > > +
> > > > + if (clk->prepare_count) {
> > > > + flags = clk_enable_lock();
> > > > + clk_core_disable(clk);
> > > > + clk_enable_unlock(flags);
> > > > + } else {
> > > > + flags = clk_enable_lock();
> > > > + clk_core_disable(parent);
> > > > + clk_enable_unlock(flags);
> > > > + clk_core_unprepare(parent);
> > > > + }
> > >
> > > Is there a reason why the clk itself can't be on when we switch
> > > parents?
> >
> > The CLK_SET_PARENT_ON only indicates that it's parent clocks should
> > be on, not itself clock.
> > And we don't want to break CLK_SET_PARENT_GATE clocks.
> > So if the clk is not prepared before, we only enable its old parent,
> > not including itself.
> >
> > > It seems that if the clk was on during the parent
> > > switch, then it should be possible to just add a flag check on
> > > both these if conditions and be done. It may be possible to
> > > change the behavior here and not enable the clk in hardware, just
> > > up the count and turn on both the parents. I'm trying to recall
> > > why we enable the clk itself across the switch.
> > >
> >
> > Seems it's needed by this fix:
> > commit f8aa0b clk: Fix race condition between clk_set_parent and clk_enable()
>
> I'm saying I don't recall why that commit needed to turn on the
> clock itself. It certainly doesn't seem to be required at first
> glance. I hope we can get away with just upping the count on the
> clock and enable both parents to avoid the race condition so that
> we don't have to worry about CLK_SET_PARENT_GATE.
>
> >
> > > > }
> > > > return ret;
> > > > }
> > > > @@ -1735,13 +1797,18 @@ static void clk_change_rate(struct clk_core *clk)
> > > > unsigned long best_parent_rate = 0;
> > > > bool skip_set_rate = false;
> > > > struct clk_core *old_parent;
> > > > + struct clk_core *parent = NULL;
> > > > + unsigned long flags;
> > > >
> > > > old_rate = clk->rate;
> > > >
> > > > - if (clk->new_parent)
> > > > + if (clk->new_parent) {
> > > > + parent = clk->new_parent;
> > > > best_parent_rate = clk->new_parent->rate;
> > > > - else if (clk->parent)
> > > > + } else if (clk->parent) {
> > > > + parent = clk->parent;
> > > > best_parent_rate = clk->parent->rate;
> > > > + }
> > > >
> > > > if (clk->new_parent && clk->new_parent != clk->parent) {
> > > > old_parent = __clk_set_parent_before(clk, clk->new_parent);
> > > > @@ -1762,6 +1829,13 @@ static void clk_change_rate(struct clk_core *clk)
> > > >
> > > > trace_clk_set_rate(clk, clk->new_rate);
> > > >
> > > > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > > > + clk_core_prepare(parent);
> > > > + flags = clk_enable_lock();
> > > > + clk_core_enable(parent);
> > > > + clk_enable_unlock(flags);
> > > > + }
> > >
> > > I can understand the case where clk_set_parent() can't switch the
> > > mux because it needs the source and destination parents to be
> > > clocking. I have that hardware design on my desk. But to change
> > > the rate of a clock? The name of the flag, CLK_SET_PARENT_ON,
> > > leads me to believe we don't really need to do this if we're
> > > changing the rate, unless we're also switching the parents. Care
> > > to explain why the hardware requires this?
> > >
> >
> > What i know is that it's designed for eliminate glitch internally.
> > Will find more information for you.
>
> On my hardware we need to have both parents on to switch the mux
> in a glitch free manner. Using the .set_rate_and_parent op we can
> change the mux value and the rate (i.e. divider) at the same time
> with one register write. And we only need to worry about having
> both parents on if the clock itself is on, which the framework
> handles for us today.
>
Our hardware needs the parent clock on for all clock settings,
parent switch, rate change and even clock enable/disable, that's
why we met issues that system will hang when disabling clock B
while its parent A is off in clk_disable_unused during booting.
The .set_rate_and_parent won't work for our issue, it's only used
if reparent during change rate. And it only enables both parents
if the clock itself is on.
But you reminder me that the MX7D clock setting registers in target
interface has clock enable/mux/divider in one regiser.
That means we do can implement .set_rate_and_parent later.
It can be an improvement.
> >
> > > If we actually do need to keep the parent clock on when the rate
> > > is switching the name of the flag could be better and not have
> > > "set parent" in the name.
> > >
> >
> > Actually it's meaning is the clock set(e.g. rate, gate/ungate)
> > needs its parent clocks on, not set parent...
> > But Yes, i fully agree the naming is a bit misleading.
> >
> > It just follows the exist CLK_SET_X convention..
> > Seems not easy find a better name.
> > What's your suggestion?
> >
> > How about CLK_OPS_PARENT_ON?
>
> Sure.
>
Okay, Will use it.
> >
> > > > +
> > > > if (!skip_set_rate && clk->ops->set_rate)
> > > > clk->ops->set_rate(clk->hw, clk->new_rate, best_parent_rate);
> > > >
> > > > @@ -1769,6 +1843,13 @@ static void clk_change_rate(struct clk_core *clk)
> > > >
> > > > clk->rate = clk_recalc(clk, best_parent_rate);
> > > >
> > > > + if (clk->flags & CLK_SET_PARENT_ON && parent) {
> > > > + flags = clk_enable_lock();
> > > > + clk_core_disable(parent);
> > > > + clk_enable_unlock(flags);
> > > > + clk_core_unprepare(parent);
> > > > + }
> > >
> > > Why not just call clk_prepare_enable()?
> >
> > Caused by two reasons initially:
> > 1) no one uses clk_prepare_enable in clk core, so i guess it may be better
> > not use it :-)
> > 2) clk_prepare_enable includes one more unneeded prepare lock acquire,
> > lines exchange efficiency.
> >
> > > Or add a clk_core
> > > specific function, clk_core_prepare_enable() that we can call
> > > here. We could put the parent pointer check in there too so that
> > > it's just
> > >
> > > if (clk->flags & CLK_SET_PARENT_ON)
> > > clk_core_prepare_enable(parent);
> >
> > I'm ok if you like this. :)
> >
>
> Yes the helper is better than calling clk_prepare_enable()
> directly.
>
Got it.
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Regards
Dong Aisheng
On Wed, May 06, 2015 at 05:01:54PM -0700, Stephen Boyd wrote:
> On 05/04, Dong Aisheng wrote:
> > On Thu, Apr 30, 2015 at 12:07:47PM -0700, Stephen Boyd wrote:
> > > On 04/15/15 07:26, Dong Aisheng wrote:
> > > > clk_core_enable is executed without &enable_clock in clk_set_parent function.
> > > > Adding it to avoid potential race condition issue.
> > > >
> > > > Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances")
> > > > Cc: Mike Turquette <[email protected]>
> > > > Cc: Stephen Boyd <[email protected]>
> > > > Signed-off-by: Dong Aisheng <[email protected]>
> > > > ---
> > >
> > > Can you please describe the race condition? From what I can tell there
> > > is not a race condition here and we've gone around on this part of the
> > > code before to fix any race conditions.
> > >
> >
> > Do you mean we do not need to acquire enable lock when execute clk_core_enable
> > in set_parent function? Can you help explain a bit more why?
> >
> > The clk doc looks to me says the enable lock should be held across calls to
> > the .enable, .disable and .is_enabled operations.
> >
> > And before the commit
> > 035a61c314eb ("clk: Make clk API return per-user struct clk instances"),
> > all the clk_enable/disable in set_parent() is executed with lock.
> >
> > A rough thinking of race condition is assuming Thread A calls
> > clk_set_parent(x, y) while Thread B calls clk_enable(x), clock x is disabled
> > but prepared initially, due to clk_core_enable in set_parent() is not
> > executed with enable clock, the clk_core_enable may be reentrant during
> > the locking time executed by B.
> > Won't this be a race condition?
> >
>
> Ah I see now. The commit text could say something like this:
>
> Before commit 035a61c314eb ("clk: Make clk API return per-user
> struct clk instances") we acquired the enable_lock in
> __clk_set_parent_{before,after}() by means of calling
> clk_enable(). After commit 035a61c314eb we use clk_core_enable()
> in place of the clk_enable(), and clk_core_enable() doesn't
> acquire the enable_lock. This opens up a race condition between
> clk_set_parent() and clk_enable().
>
> I've replaced the commit text and applied it to clk-fixes.
>
Got it.
Thanks for the change.
Regards
Dong Aisheng
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project