From patchwork Fri Aug 4 14:07:01 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Boris BREZILLON X-Patchwork-Id: 9881331 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 37D00602B8 for ; Fri, 4 Aug 2017 14:07:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 17F672823D for ; Fri, 4 Aug 2017 14:07:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0C02028972; Fri, 4 Aug 2017 14:07:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 6A4682823D for ; Fri, 4 Aug 2017 14:07:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=OFjk944ew3Um2JMVhi/17cqP2HEGDmMgO/7eHR5a530=; b=i3JsvN3OJcpdUd p3Ec9MsoQrZcEg5FMDHZpRiHedHW8e8KUyHLNroSqk4RJAuv/xrjstyFL8qqCUpawKQxHsCiNz53Y ujy+v6h/y2WBohuboNCI+fL0T/+A84baSQN0AGtrcSZO0zucuK80ETGBQAEWuooBVFrMXV9Bn1jMc BEsv/FYNlEL+yED6Fhg/0/ddiHCuPuyE6G9atgDnwyPYCIdqfatMyZt+Mey6UBPD3hmUH92cA7HR2 AziYGdL1UuKJxRvQikl1y1HCTv7Pv21VlGL+9Yd+Cj4z9r3zyXjCNb3QGvdf2qlIE62spLPQgxLfb JSnniR7+cv+DupbJds+g==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dddGM-0001uJ-93; Fri, 04 Aug 2017 14:07:38 +0000 Received: from mail.free-electrons.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dddGI-0001sk-3l for linux-rockchip@lists.infradead.org; Fri, 04 Aug 2017 14:07:36 +0000 Received: by mail.free-electrons.com (Postfix, from userid 110) id 7DB8821EB0; Fri, 4 Aug 2017 16:07:11 +0200 (CEST) Received: from bbrezillon (LStLambert-657-1-97-87.w90-63.abo.wanadoo.fr [90.63.216.87]) by mail.free-electrons.com (Postfix) with ESMTPSA id 2980221DCB; Fri, 4 Aug 2017 16:07:01 +0200 (CEST) Date: Fri, 4 Aug 2017 16:07:01 +0200 From: Boris Brezillon To: "David.Wu" Subject: Re: [v5, 22/46] pwm: rockchip: avoid glitches on already running PWMs Message-ID: <20170804160701.4d9f404d@bbrezillon> In-Reply-To: References: <1459368249-13241-23-git-send-email-boris.brezillon@free-electrons.com> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170804_070734_442961_77144E3C X-CRM114-Status: GOOD ( 28.63 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?UTF-8?B?J+m7hOa2myc=?= , Brian Norris , Heiko Stuebner , Michael Turquette , =?UTF-8?B?5byg552b?= , Stephen Boyd , Kever Yang , Doug Anderson , linux-rockchip@lists.infradead.org, Thierry Reding , =?UTF-8?B?6K645YmR576k?= , "linux-clk@vger.kernel.org" Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+patchwork-linux-rockchip=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP +Stephen, Mike and the linux-clk ML. On Fri, 4 Aug 2017 20:45:04 +0800 "David.Wu" wrote: > Hi Boris & Heiko, > > 在 2016/3/31 4:03, Boris BREZILLON 写道: > > + /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > > + pwm_get_state(pc->chip.pwms, &pstate); > > + if (!pstate.enabled) > > + clk_disable(pc->clk); > > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It > is true to close the pwm clock, and the pwm2 can't work during a while, > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for > their work. In fact, the pwm0 can not know the pwm2's status. > > So we need to get all the PWMs state in a public place where it's early > than the PWM probe, if that's the way it is. Then keep the PWM clk > enabled if theis is one PWM appears to be up and running. The place > maybe in the clock core init, like adding pwm clock as critial one. > > Another way is that we don't enable pwm clock firstly at PWM probe, > because whether or not the PWM state has been enabled in the Uboot, like > other modules, our chip default PWM clock registers are enabled at the > beginning, read the PWM registers directly to know the PWM state. Then > if the PWM state is enabled, call the enable_clk(pc->clk) to add the > clock count=1. If the PWM state is disabled, we do nothing. After all > the PWMs are probed and all modules are probed, the clock core will gate > the PWM clock if the clock count is 0, and keep clk enabled if the clock > count is not 0. > > How do you feel about it? Ouch. That's indeed hard to solve in a clean way. I may have something to suggest but I'm not sure clk maintainers will like it: what if we make clk_disable() and clk_unprepare() just decrement the refcount before the disable-unused-clks procedure has been executed (see proposed patch below)? This way all clks that have been enabled by the bootloader will stay in such state until all drivers have had a chance to retain them (IOW, call clk_prepare()+clk_enable()). BTW, I think the problem you're describing here is not unique to PWM devices, it's just that now, some PWM drivers are smart and try to keep clks enabled to prevent glitches. --->8--- From 47dcdc1bcc30b3ae1f76d33be824d2519a4dcca8 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Fri, 4 Aug 2017 15:55:49 +0200 Subject: [PATCH] clk: Keep clocks in their initial state until clk_disable_unused() is called Some drivers are briefly preparing+enabling the clock in their ->probe() hook and disable+unprepare them before leaving the function. This can be problem if a clock is shared between different devices, and one of these devices is critical to the system. If this clock is enabled/disabled by a non-critical device before the driver of the critical one had a chance to enable+prepare it, there might be a short period of time during which the critical device is not clocked. To solve this problem, we save the initial clock state (at registration time) and prevent the clock from being disabled until kernel init is done (which is when clk_disable_unused() is called). Signed-off-by: Boris Brezillon --- drivers/clk/clk.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fc58c52a26b4..3f61374a364b 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -58,6 +58,8 @@ struct clk_core { struct clk_core *new_child; unsigned long flags; bool orphan; + bool keep_enabled; + bool keep_prepared; unsigned int enable_count; unsigned int prepare_count; unsigned long min_rate; @@ -486,7 +488,7 @@ static void clk_core_unprepare(struct clk_core *core) trace_clk_unprepare(core); - if (core->ops->unprepare) + if (core->ops->unprepare && !core->keep_prepared) core->ops->unprepare(core->hw); trace_clk_unprepare_complete(core); @@ -602,7 +604,7 @@ static void clk_core_disable(struct clk_core *core) trace_clk_disable_rcuidle(core); - if (core->ops->disable) + if (core->ops->disable && !core->keep_enabled) core->ops->disable(core->hw); trace_clk_disable_complete_rcuidle(core); @@ -739,6 +741,12 @@ static void clk_unprepare_unused_subtree(struct clk_core *core) hlist_for_each_entry(child, &core->children, child_node) clk_unprepare_unused_subtree(child); + /* + * Reset the ->keep_prepared flag so that subsequent calls to + * clk_unprepare() on this clk actually unprepare it. + */ + core->keep_prepared = false; + if (core->prepare_count) return; @@ -770,6 +778,12 @@ static void clk_disable_unused_subtree(struct clk_core *core) flags = clk_enable_lock(); + /* + * Reset the ->keep_enabled flag so that subsequent calls to + * clk_disable() on this clk actually disable it. + */ + core->keep_enabled = false; + if (core->enable_count) goto unlock_out; @@ -2446,6 +2460,17 @@ static int __clk_core_init(struct clk_core *core) core->accuracy = 0; /* + * We keep track of the initial clk status to keep clks in the state + * they were left in by the bootloader until all drivers had a chance + * to keep them prepared/enabled if they need to. + */ + if (core->ops->is_prepared && !clk_ignore_unused) + core->keep_prepared = core->ops->is_prepared(core->hw); + + if (core->ops->is_enabled && !clk_ignore_unused) + core->keep_enabled = core->ops->is_enabled(core->hw); + + /* * Set clk's phase. * Since a phase is by definition relative to its parent, just * query the current clock phase, or just assume it's in phase.