Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933306AbbBBTFN (ORCPT ); Mon, 2 Feb 2015 14:05:13 -0500 Received: from mail-ie0-f180.google.com ([209.85.223.180]:61926 "EHLO mail-ie0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932442AbbBBTFK convert rfc822-to-8bit (ORCPT ); Mon, 2 Feb 2015 14:05:10 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: Steven Rostedt , "Stephen Boyd" From: Mike Turquette In-Reply-To: <20150202110033.6462d4f4@gandalf.local.home> Cc: linux-kernel@vger.kernel.org References: <1422663371-31056-1-git-send-email-sboyd@codeaurora.org> <20150202110033.6462d4f4@gandalf.local.home> Message-ID: <20150202190505.421.23196@quantum> User-Agent: alot/0.3.5 Subject: Re: [PATCH] clk: Add tracepoints for hardware operations Date: Mon, 02 Feb 2015 11:05:05 -0800 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5792 Lines: 174 Quoting Steven Rostedt (2015-02-02 08:00:33) > On Fri, 30 Jan 2015 16:16:11 -0800 > Stephen Boyd 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 > > 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. Steven, Thanks for the review. Stephen Boyd is now co-maintaining the framework by the way. Regards, Mike > > > Signed-off-by: Stephen Boyd > > --- > > 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/