[RESEND PATCH 4/8] clk: core: support clocks which requires parents on (part 2)
Dong Aisheng
dongas86 at gmail.com
Wed Jun 29 06:59:56 PDT 2016
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 at baylibre.com>
> > Cc: Stephen Boyd <sboyd at codeaurora.org>
> > Cc: Shawn Guo <shawnguo at kernel.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong at 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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the linux-arm-kernel
mailing list