Date: Mon, 2 Feb 2015 11:00:33 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: Mike Turquette <mturquette@linaro.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] clk: Add tracepoints for hardware operations
Message-ID: <20150202110033.6462d4f4@gandalf.local.home>
In-Reply-To: <1422663371-31056-1-git-send-email-sboyd@codeaurora.org>
References: <1422663371-31056-1-git-send-email-sboyd@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4769
Lines: 164

On Fri, 30 Jan 2015 16:16:11 -0800
Stephen Boyd <sboyd@codeaurora.org> wrote:

> It's useful to have tracepoints around operations that change the
> hardware state so that we can debug clock hardware performance
> and operations. Four basic types of events are supported: on/off
> events for enable, disable, prepare, unprepare that only record
> an event and a clock name, rate changing events for
> clk_set_{min_,max_}rate{_range}(), phase changing events for
> clk_set_phase() and parent changing events for clk_set_parent().
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>

I don't see anything wrong with the implementation of the tracepoints.
Now whether or not they are useful is up to the clk maintainer to
decide.

> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/clk/clk.c          |  32 ++++++++
>  include/trace/events/clk.h | 198 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 230 insertions(+)
>  create mode 100644 include/trace/events/clk.h
> 


>  unlock_out:
> @@ -861,9 +868,12 @@ static void clk_core_unprepare(struct clk_core *clk)
>  
>  	WARN_ON(clk->enable_count > 0);
>  
> +	trace_clk_unprepare(clk);
> +
>  	if (clk->ops->unprepare)
>  		clk->ops->unprepare(clk->hw);
>  
> +	trace_clk_unprepare_complete(clk);
>  	clk_core_unprepare(clk->parent);

I guess you do not care about the clk_core_unprepare time.

>  }
>  
> @@ -901,6 +911,8 @@ static int clk_core_prepare(struct clk_core *clk)
>  		if (ret)
>  			return ret;
>  
> +		trace_clk_prepare(clk);
> +
>  		if (clk->ops->prepare) {
>  			ret = clk->ops->prepare(clk->hw);
>  			if (ret) {
> @@ -908,6 +920,8 @@ static int clk_core_prepare(struct clk_core *clk)
>  				return ret;
>  			}
>  		}
> +
> +		trace_clk_prepare_complete(clk);

I'm curious to why you do not put the tracepoint within the if
statement, and only show the tracepoints if the clock prepare is
actually called. Also, if you exit out with that return, will you tools
be OK with seeing the clk_prepare but not the clk_prepare_complete?


>  	}
>  
>  	clk->prepare_count++;
> @@ -953,9 +967,13 @@ static void clk_core_disable(struct clk_core *clk)
>  	if (--clk->enable_count > 0)
>  		return;
>  
> +	trace_clk_disable(clk);
> +
>  	if (clk->ops->disable)
>  		clk->ops->disable(clk->hw);
>  
> +	trace_clk_disable_complete(clk);
> +
>  	clk_core_disable(clk->parent);
>  }
>  
> @@ -1008,6 +1026,7 @@ static int clk_core_enable(struct clk_core *clk)
>  		if (ret)
>  			return ret;
>  
> +		trace_clk_enable(clk);
>  		if (clk->ops->enable) {
>  			ret = clk->ops->enable(clk->hw);
>  			if (ret) {
> @@ -1015,6 +1034,7 @@ static int clk_core_enable(struct clk_core *clk)
>  				return ret;
>  			}
>  		}
> +		trace_clk_enable_complete(clk);

Same here.

-- Steve

>  	}
>  
>  	clk->enable_count++;
> @@ -1383,6 +1403,8 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *clk,
>  	unsigned long flags;
>  	struct clk_core *old_parent = clk->parent;
>  
> +	trace_clk_set_parent(clk, parent);
> +
>  	/*
>  	 * Migrate prepare state between parents and prevent race with
>  	 * clk_enable().
> @@ -1427,6 +1449,8 @@ static void __clk_set_parent_after(struct clk_core *core,
>  		clk_core_disable(old_parent);
>  		clk_core_unprepare(old_parent);
>  	}
> +
> +	trace_clk_set_parent_complete(core, parent);
>  }
>  
>  static int __clk_set_parent(struct clk_core *clk, struct clk_core *parent,
> @@ -1663,6 +1687,8 @@ static void clk_change_rate(struct clk_core *clk)
>  	else if (clk->parent)
>  		best_parent_rate = clk->parent->rate;
>  
> +	trace_clk_set_rate(clk, clk->new_rate);
> +
>  	if (clk->new_parent && clk->new_parent != clk->parent) {
>  		old_parent = __clk_set_parent_before(clk, clk->new_parent);
>  
> @@ -1681,6 +1707,8 @@ static void clk_change_rate(struct clk_core *clk)
>  	if (!skip_set_rate && clk->ops->set_rate)
>  		clk->ops->set_rate(clk->hw, clk->new_rate, best_parent_rate);
>  
> +	trace_clk_set_rate_complete(clk, clk->new_rate);
> +
>  	clk->rate = clk_recalc(clk, best_parent_rate);
>  
>  	if (clk->notifier_count && old_rate != clk->rate)
> @@ -2081,6 +2109,8 @@ int clk_set_phase(struct clk *clk, int degrees)
>  
>  	clk_prepare_lock();
>  
> +	trace_clk_set_phase(clk->core, degrees);
> +
>  	if (!clk->core->ops->set_phase)
>  		goto out_unlock;
>  
> @@ -2090,6 +2120,8 @@ int clk_set_phase(struct clk *clk, int degrees)
>  		clk->core->phase = degrees;
>  
>  out_unlock:
> +	trace_clk_set_phase_complete(clk->core, degrees);
> +
>  	clk_prepare_unlock();
>  
>  out:

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/