From patchwork Mon Sep 7 16:54:03 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Magnus Damm X-Patchwork-Id: 7137381 X-Patchwork-Delegate: geert@linux-m68k.org Return-Path: X-Original-To: patchwork-linux-sh@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 E79DE9F380 for ; Mon, 7 Sep 2015 16:54:10 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 741C320618 for ; Mon, 7 Sep 2015 16:54:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EAFBB20617 for ; Mon, 7 Sep 2015 16:54:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751287AbbIGQyH (ORCPT ); Mon, 7 Sep 2015 12:54:07 -0400 Received: from mail-la0-f45.google.com ([209.85.215.45]:35194 "EHLO mail-la0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750999AbbIGQyG (ORCPT ); Mon, 7 Sep 2015 12:54:06 -0400 Received: by lagj9 with SMTP id j9so55503905lag.2; Mon, 07 Sep 2015 09:54:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=NEqbz3WQI5f0mJ+1yL3JxXKMY14YQwGl97dyiq+gjZo=; b=RRdwfo10by+yUf9uXQT36hO3EKza4SAF9d/P2xhr5n00bNU7tNEP1Q0UfM9E3YyrDk sqvexwbrz9fixC5QuN51mBIRtUrtKTGJyliri95gIIZ0CtXgxmfqDFY3sHxUafjKLbdy 6/G+WYEQytdioDPLQNgL+MyDicXg+M3pcVLzN+mbwQS3GdFLOP3KKPC2Sf0FiCw//Mcs 5ZPscgwZWPPbmY6w9ZTAaxSpUkKGqWqrsqTGzGBNrbyvdWnlkrq9as9TCTVIvIEL9y8k Vz8TyPabbs/Zu/eTiERN6wlra9vKpX44EVGC4bwtI/PNhd6jA9xWs8FUm6hVam4n4gbv i4Mw== MIME-Version: 1.0 X-Received: by 10.112.199.230 with SMTP id jn6mr1229156lbc.12.1441644843147; Mon, 07 Sep 2015 09:54:03 -0700 (PDT) Received: by 10.114.67.162 with HTTP; Mon, 7 Sep 2015 09:54:03 -0700 (PDT) In-Reply-To: References: Date: Tue, 8 Sep 2015 01:54:03 +0900 Message-ID: Subject: Re: Regression due to dropping "clock-output-names" (was: Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support) From: Magnus Damm To: Geert Uytterhoeven Cc: Michael Turquette , Stephen Boyd , linux-clk , Kuninori Morimoto , Gaku Inami , Linux-sh list , Simon Horman , Laurent Pinchart Sender: linux-sh-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Geert, Mike, Stephen, all, On Thu, Sep 3, 2015 at 5:22 PM, Geert Uytterhoeven wrote: > Ping? Btw, I've just sent off the V7 series which still has this issue: [PATCH v7 00/05] Renesas R-Car Gen3 CPG support V7 From my point of view it looks like some core CCF changes are needed to allow us to drop "clock-output-names" for the CPG node. I've written some ugly prototype code to patch things together and I've pasted it in as a proof-of-concent patch below. My apologies in advance if gmail mangled the inline patch somehow... > On Mon, Aug 31, 2015 at 5:56 PM, Geert Uytterhoeven > wrote: >> On Mon, Aug 31, 2015 at 2:49 PM, Magnus Damm wrote: >>> From: Gaku Inami >>> >>> This V5 patch adds initial CPG support for R-Car Generation 3 and in >>> particular the R8A7795 SoC. >>> >>> The R-Car Gen3 clock hardware has a register write protection feature that >>> needs to be shared between the CPG function needs to be shared between >>> the CPG and MSTP hardware somehow. So far this feature is simply ignored. >>> >>> Signed-off-by: Gaku Inami >>> Signed-off-by: Kuninori Morimoto >>> Signed-off-by: Magnus Damm >>> --- >>> >>> Changes since V4: (Magnus Damm ) >>> - Simplified clks array handling - thanks Geert! >>> - Updated th DT binding documentation to reflect latest state >>> - of_property_count_strings() -> of_property_count_u32_elems() fix >>> >>> Changes since V3: (Magnus Damm ) >>> - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!! >>> - Major things like syscon and driver model require more discussion. >>> - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set. >>> >>> Changes since V2: (Magnus Damm ) >>> - Reworked driver to rely on clock index instead of strings. >>> - Dropped use of "clock-output-names". >> >> Unfortunately dropping "clock-output-names" causes a regression: it turns >> out all fixed-factor clocks that have <&cpg_clocks R8A7795_CLK_*> as a parent >> have lost their parent clock, and have rate zero. >> >> E.g. instead of >> >> clock enable_cnt prepare_cnt rate >> accuracy phase >> ---------------------------------------------------------------------------------------- >> extal 1 1 16666600 >> 50000 0 >> main 1 1 8333300 >> 50000 0 >> pll1 1 2 799996800 >> 50000 0 >> cl 0 0 16666600 >> 50000 0 >> s3 1 1 133332800 >> 50000 0 >> s3d4 1 2 33333200 >> 50000 0 >> mstp3_clks.10 2 2 33333200 >> 50000 0 >> >> /sys/kernel/debug/clk/clk_summary now has: >> >> clock enable_cnt prepare_cnt rate >> accuracy phase >> ---------------------------------------------------------------------------------------- >> extal 0 0 16666600 >> 50000 0 >> main 0 0 8333300 >> 50000 0 >> pll1 0 0 799996800 >> 50000 0 >> cl 0 0 0 >> 0 0 >> s3 1 1 0 >> 0 0 >> s3d4 1 2 0 >> 0 0 >> mstp3_clks.10 2 2 0 >> 0 0 >> >> Surprisingly, the system still works. >> >> Several clock drivers (e.g. fixed-factor-clock) use of_clk_get_parent_name() >> to find the name of the parent clock. >> In the absence of "clock-output-names", this returns the name of the device >> node of the parent's clock. Which is always "cpg_clocks", and no longer the >> name of the clock matching the index. >> >> I believe the same issue is present for MSTP clocks, but there we don't have >> children clocks, so it doesn't manifest itself (yet). >> >> A quick workaround is to just re-add the clock-output-names to r8a7795.dtsi: >> >> clock-output-names = >> "main", "pll0", "pll1","pll2", >> "pll3", "pll4"; >> >> Hence it seems like dropping "clock-output-names" for clocks with multiple >> outputs is not such a good idea? >> >> See also my question in "Re: [PATCH 3/4 v3][RFC] arm64: renesas: Add initial >> r8a7795 SoC support" (http://www.spinics.net/lists/linux-sh/msg44609.html): >> >> | BTW, was "dropping clock-output-names" a general comment for all clock >> | drivers, or specific to the SoC's Clock Pulse Generator? >> | >> | For e.g. "fixed-factor-clock", the driver falls back to using the node's >> | name if "clock-output-names" is not present. >> | >> | But e.g. for MSTP clocks, that can't be done, as the clocks wouldn't have >> | names in that case (single node with multiple clocks). Unless we start >> | fabricating them from the node name and the indices." >> >> For MSTP, we started fabricating them, but that doesn't solve the >> of_clk_get_parent_name() issue. >> >> Thanks for your comments! Thanks for making this very useful summary. It made it relatively easy for me to understand what was going on. From my side I've noticed exactly the same thing as you describe. So in case we have more than one clock provided by a single node the of_clk_get_parent_name() function will not work as expected unless clock-output-names is provided. More or less all of the main clock drivers for renesas hardware is built using a single node with multiple indices, and today we use clock-output-names so we have no issues for existing platforms. For the upcoming R-Car Gen3 SoC we were asked to get rid of clock-output-names if possible, but it seems that is difficult to do without changing core CCF code. The patch below is my proporsal how to fix the issue. It needs to be reworked to handle memory allocation properly, but hopefully there are better ways to fix this. The main portion of the patch below is based around a new function called of_clk_get_name(). The idea is that it will check a certain node if the clock-indices property is present together with clock-output-names. In case clock-output-names is set it will make use of that. If no clock-output-names is provided and no clock-indices then the node name is used as-is, but if clock-indices is set then a "%s.%u" name is generated based on node name and clock-index. Included in the patch is also some debug code and minor changes to make use of of_clk_get_name() from of_clk_get_parent_name() and a couple of Renesas-specific drivers. With this patch the debugfs clock frequency propagation is unbroken again, but unfortunately we now have memory leaks instead. =) The prototype is only written to highlight what that the problem is. Not for upstream merge. Not-yet-Signed-off-by: Magnus Damm --- drivers/clk/clk-fixed-factor.c | 3 ++ drivers/clk/clk.c | 46 ++++++++++++++++++++++------------ drivers/clk/shmobile/clk-mstp.c | 3 -- drivers/clk/shmobile/clk-rcar-gen3.c | 13 +-------- include/linux/clk-provider.h | 2 + 5 files changed, 39 insertions(+), 28 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- 0001/drivers/clk/clk-fixed-factor.c +++ work/drivers/clk/clk-fixed-factor.c 2015-09-07 21:49:36.472366518 +0900 @@ -82,6 +82,9 @@ struct clk *clk_register_fixed_factor(st if (!fix) return ERR_PTR(-ENOMEM); + printk("xxxx name %s parent name %s %i %i\n", name, parent_name, + mult, div); + /* struct clk_fixed_factor assignments */ fix->mult = mult; fix->div = div; --- 0001/drivers/clk/clk.c +++ work/drivers/clk/clk.c 2015-09-07 22:08:32.472366518 +0900 @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic } EXPORT_SYMBOL_GPL(of_clk_get_parent_count); -const char *of_clk_get_parent_name(struct device_node *np, int index) +const char *of_clk_get_name(struct device_node *np, int index) { - struct of_phandle_args clkspec; struct property *prop; const char *clk_name; const __be32 *vp; u32 pv; - int rc; int count; + bool has_indices = false; if (index < 0) return NULL; - rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, - &clkspec); - if (rc) - return NULL; - - index = clkspec.args_count ? clkspec.args[0] : 0; count = 0; /* if there is an indices property, use it to transfer the index * specified into an array offset for the clock-output-names property. */ - of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) { + of_property_for_each_u32(np, "clock-indices", prop, vp, pv) { + has_indices = true; if (index == pv) { index = count; break; @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc count++; } - if (of_property_read_string_index(clkspec.np, "clock-output-names", - index, - &clk_name) < 0) - clk_name = clkspec.np->name; + if (of_property_read_string_index(np, "clock-output-names", index, + &clk_name) < 0) { + if (has_indices) + return kasprintf(GFP_KERNEL, "%s.%u", np->name, index); + else + return kstrdup_const(np->name, GFP_KERNEL); + } + + return kstrdup_const(clk_name, GFP_KERNEL); +} +EXPORT_SYMBOL_GPL(of_clk_get_name); + +const char *of_clk_get_parent_name(struct device_node *np, int index) +{ + struct of_phandle_args clkspec; + const char *clk_name = NULL; + int rc; + + if (index < 0) + return NULL; - of_node_put(clkspec.np); + rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, + &clkspec); + if (!rc) { + clk_name = of_clk_get_name(clkspec.np, clkspec.args_count ? + clkspec.args[0] : 0); + of_node_put(clkspec.np); + } return clk_name; } EXPORT_SYMBOL_GPL(of_clk_get_parent_name); --- 0017/drivers/clk/shmobile/clk-mstp.c +++ work/drivers/clk/shmobile/clk-mstp.c 2015-09-07 22:04:50.942366518 +0900 @@ -216,8 +216,7 @@ static void __init cpg_mstp_clocks_init( if (of_property_read_string_index(np, "clock-output-names", i, &name) < 0) - allocated_name = name = kasprintf(GFP_KERNEL, "%s.%u", - np->name, clkidx); + allocated_name = name = of_clk_get_name(np, clkidx); clks[clkidx] = cpg_mstp_clock_register(name, parent_name, clkidx, group); --- 0020/drivers/clk/shmobile/clk-rcar-gen3.c +++ work/drivers/clk/shmobile/clk-rcar-gen3.c 2015-09-07 21:41:38.502366518 +0900 @@ -26,15 +26,6 @@ #define RCAR_GEN3_CLK_PLL4 5 #define RCAR_GEN3_CLK_NR 6 -static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = { - [RCAR_GEN3_CLK_MAIN] = "main", - [RCAR_GEN3_CLK_PLL0] = "pll0", - [RCAR_GEN3_CLK_PLL1] = "pll1", - [RCAR_GEN3_CLK_PLL2] = "pll2", - [RCAR_GEN3_CLK_PLL3] = "pll3", - [RCAR_GEN3_CLK_PLL4] = "pll4", -}; - struct rcar_gen3_cpg { struct clk_onecell_data data; void __iomem *reg; @@ -136,7 +127,7 @@ rcar_gen3_cpg_register_clk(struct device const struct cpg_pll_config *config, unsigned int gen3_clk) { - const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN]; + const char *parent_name = of_clk_get_name(np, RCAR_GEN3_CLK_MAIN); unsigned int mult = 1; unsigned int div = 1; u32 value; @@ -177,7 +168,7 @@ rcar_gen3_cpg_register_clk(struct device return ERR_PTR(-EINVAL); } - return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk], + return clk_register_fixed_factor(NULL, of_clk_get_name(np, gen3_clk), parent_name, 0, mult, div); } --- 0001/include/linux/clk-provider.h +++ work/include/linux/clk-provider.h 2015-09-07 21:35:54.332366518 +0900 @@ -665,6 +665,8 @@ struct clk *of_clk_src_onecell_get(struc int of_clk_get_parent_count(struct device_node *np); int of_clk_parent_fill(struct device_node *np, const char **parents, unsigned int size); + +const char *of_clk_get_name(struct device_node *np, int index); const char *of_clk_get_parent_name(struct device_node *np, int index); void of_clk_init(const struct of_device_id *matches);