Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932452AbbBBQAj (ORCPT ); Mon, 2 Feb 2015 11:00:39 -0500 Received: from smtprelay0097.hostedemail.com ([216.40.44.97]:48969 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753950AbbBBQAi (ORCPT ); Mon, 2 Feb 2015 11:00:38 -0500 X-Session-Marker: 6E657665747340676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::,RULES_HIT:41:355:379:421:541:599:800:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1535:1544:1593:1594:1605:1711:1730:1747:1777:1792:2194:2199:2393:2553:2559:2562:2693:3138:3139:3140:3141:3142:3622:3865:3866:3867:3868:3871:3872:3873:3874:4250:4605:5007:6119:6261:7875:7903:8666:10004:10848:10967:11026:11232:11658:11914:12043:12291:12296:12438:12517:12519:12555:12663:12683:12740:13151:13228:14096:14097:21080,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0 X-HE-Tag: fruit39_79904663df73c X-Filterd-Recvd-Size: 5531 Date: Mon, 2 Feb 2015 11:00:33 -0500 From: Steven Rostedt To: Stephen Boyd Cc: Mike Turquette , 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> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4769 Lines: 164 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. > 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/