| Message ID | 1461144880-8724-5-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dong, Quoting Dong Aisheng (2016-04-20 02:34:36) > 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 adding flag CLK_OPS_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 patch part 2 fixes set clock rate and set parent while its parent > is off. The most special case is for set_parent() operation which requires > all parents including both old and new one to be enabled at the same time > during the operation. > > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Shawn Guo <shawnguo@kernel.org> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++----------------- > 1 file changed, 28 insertions(+), 17 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 1f054d1a..d6cef61 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1174,7 +1174,7 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *core, > struct clk_core *old_parent = core->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 > @@ -1189,13 +1189,16 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *core, > * hardware and software states. > * > * See also: Comment for clk_set_parent() below. > + * > + * 2. enable parent clocks for CLK_OPS_PARENT_ON clock > */ > - if (core->prepare_count) { > - clk_core_prepare(parent); > - flags = clk_enable_lock(); > - clk_core_enable(parent); > - clk_core_enable(core); > - clk_enable_unlock(flags); > + if (core->prepare_count || core->flags & CLK_OPS_PARENT_ON) { > + clk_core_prepare_enable(parent); > + if (core->prepare_count) > + clk_core_enable_lock(core); > + else > + clk_core_prepare_enable(old_parent); It took me a few seconds to figure out what I was looking at. The set_parent stuff is tricky enough already ... I think we should keep the state machine logic very simple. How about: /* enable old_parent & parent if CLK_OPS_PARENT_ENABLE is set */ if (core->flags & CLK_OPS_PARENT_ENABLE) { clk_core_prepare_enable(old_parent); clk_core_prepare_enable(parent); } /* migrate prepare count if > 0 */ if (core->prepare_count) { clk_core_prepare_enable(parent); clk_core_enable_lock(core); } > + > } > > /* update the clk tree topology */ > @@ -1210,18 +1213,16 @@ static void __clk_set_parent_after(struct clk_core *core, > 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 (core->prepare_count) { > - flags = clk_enable_lock(); > - clk_core_disable(core); > - clk_core_disable(old_parent); > - clk_enable_unlock(flags); > - clk_core_unprepare(old_parent); > + if (core->prepare_count || core->flags & CLK_OPS_PARENT_ON) { > + clk_core_disable_unprepare(old_parent); > + if (core->prepare_count) > + clk_core_disable_lock(core); > + else > + clk_core_disable_unprepare(parent); Similarly, a more readable solution is: /* * 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_lock(core); clk_core_disable_unprepare(old_parent); /* re-balance ref counting if CLK_OPS_PARENT_ENABLE is set */ if (core->flags & CLK_OPS_PARENT_ENABLE) { clk_core_disable_unprepare(parent); clk_core_disable_unprepare(old_parent); } > } > } > > @@ -1468,13 +1469,17 @@ static void clk_change_rate(struct clk_core *core) > unsigned long best_parent_rate = 0; > bool skip_set_rate = false; > struct clk_core *old_parent; > + struct clk_core *parent = NULL; > > old_rate = core->rate; > > - if (core->new_parent) > + if (core->new_parent) { > + parent = core->new_parent; > best_parent_rate = core->new_parent->rate; > - else if (core->parent) > + } else if (core->parent) { > + parent = core->parent; > best_parent_rate = core->parent->rate; > + } > > if (core->flags & CLK_SET_RATE_UNGATE) { > unsigned long flags; > @@ -1504,6 +1509,9 @@ static void clk_change_rate(struct clk_core *core) > > trace_clk_set_rate(core, core->new_rate); > > + if (core->flags & CLK_OPS_PARENT_ON) > + clk_core_prepare_enable(parent); > + > if (!skip_set_rate && core->ops->set_rate) > core->ops->set_rate(core->hw, core->new_rate, best_parent_rate); > > @@ -1520,6 +1528,9 @@ static void clk_change_rate(struct clk_core *core) > clk_core_unprepare(core); > } > > + if (core->flags & CLK_OPS_PARENT_ON) > + clk_core_disable_unprepare(parent); > + This part looks good to me. Regards, Mike > if (core->notifier_count && old_rate != core->rate) > __clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate); > > -- > 1.9.1 >
Hi Mike, On Fri, Jun 24, 2016 at 04:52:59PM -0700, Michael Turquette wrote: > Hi Dong, > > Quoting Dong Aisheng (2016-04-20 02:34:36) > > 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 adding flag CLK_OPS_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 patch part 2 fixes set clock rate and set parent while its parent > > is off. The most special case is for set_parent() operation which requires > > all parents including both old and new one to be enabled at the same time > > during the operation. > > > > Cc: Michael Turquette <mturquette@baylibre.com> > > Cc: Stephen Boyd <sboyd@codeaurora.org> > > Cc: Shawn Guo <shawnguo@kernel.org> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++----------------- > > 1 file changed, 28 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 1f054d1a..d6cef61 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -1174,7 +1174,7 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *core, > > struct clk_core *old_parent = core->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 > > @@ -1189,13 +1189,16 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *core, > > * hardware and software states. > > * > > * See also: Comment for clk_set_parent() below. > > + * > > + * 2. enable parent clocks for CLK_OPS_PARENT_ON clock > > */ > > - if (core->prepare_count) { > > - clk_core_prepare(parent); > > - flags = clk_enable_lock(); > > - clk_core_enable(parent); > > - clk_core_enable(core); > > - clk_enable_unlock(flags); > > + if (core->prepare_count || core->flags & CLK_OPS_PARENT_ON) { > > + clk_core_prepare_enable(parent); > > + if (core->prepare_count) > > + clk_core_enable_lock(core); > > + else > > + clk_core_prepare_enable(old_parent); > > It took me a few seconds to figure out what I was looking at. The > set_parent stuff is tricky enough already ... I think we should keep the > state machine logic very simple. How about: > > /* enable old_parent & parent if CLK_OPS_PARENT_ENABLE is set */ > if (core->flags & CLK_OPS_PARENT_ENABLE) { > clk_core_prepare_enable(old_parent); > clk_core_prepare_enable(parent); > } > > /* migrate prepare count if > 0 */ > if (core->prepare_count) { > clk_core_prepare_enable(parent); > clk_core_enable_lock(core); > } > Great suggestion! That looks much clearer. Will update it in V2. Thanks Regards Dong Aisheng > > + > > } > > > > /* update the clk tree topology */ > > @@ -1210,18 +1213,16 @@ static void __clk_set_parent_after(struct clk_core *core, > > 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 (core->prepare_count) { > > - flags = clk_enable_lock(); > > - clk_core_disable(core); > > - clk_core_disable(old_parent); > > - clk_enable_unlock(flags); > > - clk_core_unprepare(old_parent); > > + if (core->prepare_count || core->flags & CLK_OPS_PARENT_ON) { > > + clk_core_disable_unprepare(old_parent); > > + if (core->prepare_count) > > + clk_core_disable_lock(core); > > + else > > + clk_core_disable_unprepare(parent); > > Similarly, a more readable solution is: > > /* > * 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_lock(core); > clk_core_disable_unprepare(old_parent); > > /* re-balance ref counting if CLK_OPS_PARENT_ENABLE is set */ > if (core->flags & CLK_OPS_PARENT_ENABLE) { > clk_core_disable_unprepare(parent); > clk_core_disable_unprepare(old_parent); > } > > > } > > } > > > > @@ -1468,13 +1469,17 @@ static void clk_change_rate(struct clk_core *core) > > unsigned long best_parent_rate = 0; > > bool skip_set_rate = false; > > struct clk_core *old_parent; > > + struct clk_core *parent = NULL; > > > > old_rate = core->rate; > > > > - if (core->new_parent) > > + if (core->new_parent) { > > + parent = core->new_parent; > > best_parent_rate = core->new_parent->rate; > > - else if (core->parent) > > + } else if (core->parent) { > > + parent = core->parent; > > best_parent_rate = core->parent->rate; > > + } > > > > if (core->flags & CLK_SET_RATE_UNGATE) { > > unsigned long flags; > > @@ -1504,6 +1509,9 @@ static void clk_change_rate(struct clk_core *core) > > > > trace_clk_set_rate(core, core->new_rate); > > > > + if (core->flags & CLK_OPS_PARENT_ON) > > + clk_core_prepare_enable(parent); > > + > > if (!skip_set_rate && core->ops->set_rate) > > core->ops->set_rate(core->hw, core->new_rate, best_parent_rate); > > > > @@ -1520,6 +1528,9 @@ static void clk_change_rate(struct clk_core *core) > > clk_core_unprepare(core); > > } > > > > + if (core->flags & CLK_OPS_PARENT_ON) > > + clk_core_disable_unprepare(parent); > > + > > This part looks good to me. > > Regards, > Mike > > > if (core->notifier_count && old_rate != core->rate) > > __clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate); > > > > -- > > 1.9.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 1f054d1a..d6cef61 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1174,7 +1174,7 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *core, struct clk_core *old_parent = core->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 @@ -1189,13 +1189,16 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *core, * hardware and software states. * * See also: Comment for clk_set_parent() below. + * + * 2. enable parent clocks for CLK_OPS_PARENT_ON clock */ - if (core->prepare_count) { - clk_core_prepare(parent); - flags = clk_enable_lock(); - clk_core_enable(parent); - clk_core_enable(core); - clk_enable_unlock(flags); + if (core->prepare_count || core->flags & CLK_OPS_PARENT_ON) { + clk_core_prepare_enable(parent); + if (core->prepare_count) + clk_core_enable_lock(core); + else + clk_core_prepare_enable(old_parent); + } /* update the clk tree topology */ @@ -1210,18 +1213,16 @@ static void __clk_set_parent_after(struct clk_core *core, 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 (core->prepare_count) { - flags = clk_enable_lock(); - clk_core_disable(core); - clk_core_disable(old_parent); - clk_enable_unlock(flags); - clk_core_unprepare(old_parent); + if (core->prepare_count || core->flags & CLK_OPS_PARENT_ON) { + clk_core_disable_unprepare(old_parent); + if (core->prepare_count) + clk_core_disable_lock(core); + else + clk_core_disable_unprepare(parent); } } @@ -1468,13 +1469,17 @@ static void clk_change_rate(struct clk_core *core) unsigned long best_parent_rate = 0; bool skip_set_rate = false; struct clk_core *old_parent; + struct clk_core *parent = NULL; old_rate = core->rate; - if (core->new_parent) + if (core->new_parent) { + parent = core->new_parent; best_parent_rate = core->new_parent->rate; - else if (core->parent) + } else if (core->parent) { + parent = core->parent; best_parent_rate = core->parent->rate; + } if (core->flags & CLK_SET_RATE_UNGATE) { unsigned long flags; @@ -1504,6 +1509,9 @@ static void clk_change_rate(struct clk_core *core) trace_clk_set_rate(core, core->new_rate); + if (core->flags & CLK_OPS_PARENT_ON) + clk_core_prepare_enable(parent); + if (!skip_set_rate && core->ops->set_rate) core->ops->set_rate(core->hw, core->new_rate, best_parent_rate); @@ -1520,6 +1528,9 @@ static void clk_change_rate(struct clk_core *core) clk_core_unprepare(core); } + if (core->flags & CLK_OPS_PARENT_ON) + clk_core_disable_unprepare(parent); + if (core->notifier_count && old_rate != core->rate) __clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate);
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 adding flag CLK_OPS_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 patch part 2 fixes set clock rate and set parent while its parent is off. The most special case is for set_parent() operation which requires all parents including both old and new one to be enabled at the same time during the operation. Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Shawn Guo <shawnguo@kernel.org> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-)