From patchwork Thu Apr 14 00:19:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 8830311 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id B59429F3A0 for ; Thu, 14 Apr 2016 00:21:13 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id C07942021A for ; Thu, 14 Apr 2016 00:21:12 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CBD962020F for ; Thu, 14 Apr 2016 00:21:11 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1aqV0V-0005Lf-07; Thu, 14 Apr 2016 00:19:39 +0000 Received: from smtp.codeaurora.org ([198.145.29.96]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1aqV0R-0005I2-V3 for linux-arm-kernel@lists.infradead.org; Thu, 14 Apr 2016 00:19:36 +0000 Received: from smtp.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id AE3A760D79; Thu, 14 Apr 2016 00:19:14 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 1000) id A0A7360DB8; Thu, 14 Apr 2016 00:19:14 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Received: from localhost (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: sboyd@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id E120960215; Thu, 14 Apr 2016 00:19:13 +0000 (UTC) Date: Wed, 13 Apr 2016 17:19:13 -0700 From: Stephen Boyd To: Sjoerd Simons Subject: Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks Message-ID: <20160414001913.GG14441@codeaurora.org> References: <1459947173-6664-1-git-send-email-sjoerd.simons@collabora.co.uk> <1459949826.23936.19.camel@collabora.co.uk> <20160407232155.GH18567@codeaurora.org> <1460112654.31245.20.camel@collabora.co.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1460112654.31245.20.camel@collabora.co.uk> User-Agent: Mutt/1.5.21 (2010-09-15) X-Virus-Scanned: ClamAV using ClamSMTP X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160413_171936_064667_06C7360C X-CRM114-Status: GOOD ( 26.97 ) X-Spam-Score: -2.9 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "devicetree@vger.kernel.org" , Michael Turquette , "linux-kernel@vger.kernel.org" , linux-renesas-soc@vger.kernel.org, Simon Horman , Geert Uytterhoeven , linux-clk , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 04/08, Sjoerd Simons wrote: > On Thu, 2016-04-07 at 16:21 -0700, Stephen Boyd wrote: > > On 04/06, Sjoerd Simons wrote: > > > > > > Though even so I'm not sure what the convention is for clocks like > > > these, the r8a7791.dtsi is inconsistent, as some are disabled while > > > others (e.g. the audio clocks) are 0hz. Would be good to get some > > > input > > > on that regardless. > > > > > What's the question here? > > So the question is how to model unconnected external clocks in device- > tree. > > The dtsi we're loooking at has (in pseudo dt): > > device { >   clock-names = "internal", "external"; >   clocks = <&internal, &external> > }; > > external { >   compatible = "fixed-clock"; >   clock-frequency = <12345>; >   status = "disabled"; > }; > > Before 3e5dd6f6e690048d ("clk: Ignore disabled DT clock providers") > this apparently worked. Afterwards drivers getting all the clocks would > fail to probe with -EPROBE_DEFER. > > Judging by your comment I assume this way of modelling it is broken > (and the behaviour caused by the patch is correct)?  > > And as a follow-up, is modelling unconnected clocks as enabled with a > frequency of 0hz as my proposed patch does seen as the right way of > doing things? > Right. In the case where the external clk is populated, I imagine there would be a DT node describing it with the clock-frequency property if it's a fixed rate clk. If the clk is not populated on the board, then we would have a "ground" clk node that has a frequency of 0. This way, if we have some mux clk that is default connected to the external clk but can switch to some internal clk it isn't orphaned forever. This is actually a problem for orphan clk deferral right now. My head starts to spin when we consider something like expansion boards that have clk pins on them though. Hopefully for things like that, we can populate clks with DT overlays and then change the root hierarchy of the clk tree by swapping out the "ground" clk for some real clk on the expansion board. The usage of strings to describe the clk tree is probably going to get us here though. Fun! If we can't do this DT design because of backwards compatibility concerns, then perhaps we need to expand the core to return a fixed rate of 0 clk whenever clk_get() is called on a provider that's status = "disabled"? Here's an untested patch to show that idea. ---8<---- diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fb74dc1f7520..19c3777b1cea 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3085,6 +3085,17 @@ int of_clk_parent_fill(struct device_node *np, const char **parents, } EXPORT_SYMBOL_GPL(of_clk_parent_fill); +static void init_disabled_clk_provider(struct device_node *np) +{ + struct clk *clk; + + clk = clk_register_fixed_rate(NULL, of_node_full_name(np), NULL, 0, 0); + if (IS_ERR(clk)) + return; + + of_clk_add_provider(np, of_clk_src_simple_get, clk); +} + struct clock_provider { of_clk_init_cb_t clk_init_cb; struct device_node *np; @@ -3150,9 +3161,6 @@ void __init of_clk_init(const struct of_device_id *matches) for_each_matching_node_and_match(np, matches, &match) { struct clock_provider *parent; - if (!of_device_is_available(np)) - continue; - parent = kzalloc(sizeof(*parent), GFP_KERNEL); if (!parent) { list_for_each_entry_safe(clk_provider, next, @@ -3165,7 +3173,18 @@ void __init of_clk_init(const struct of_device_id *matches) return; } - parent->clk_init_cb = match->data; + /* + * Sometimes DT nodes are status = "disabled" but they're used + * by other clk providers. In that case we make the disabled + * provider return fixed rate clks with a frequency of 0 so + * that nothing is orphaned and drivers can still get all + * their clks. + */ + if (!of_device_is_available(np)) { + parent->clk_init_cb = init_disabled_clk_provider; + } else { + parent->clk_init_cb = match->data; + } parent->np = of_node_get(np); list_add_tail(&parent->node, &clk_provider_list); }