From patchwork Wed Nov 6 08:19:48 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Magnus Damm X-Patchwork-Id: 3145401 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.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 16D9F9F3C4 for ; Wed, 6 Nov 2013 08:20:35 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A82492054B for ; Wed, 6 Nov 2013 08:20:33 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 33AD620513 for ; Wed, 6 Nov 2013 08:20:32 +0000 (UTC) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VdyLc-0002AR-BI; Wed, 06 Nov 2013 08:20:20 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1VdyLZ-0007Hx-RV; Wed, 06 Nov 2013 08:20:17 +0000 Received: from mail-lb0-x22b.google.com ([2a00:1450:4010:c04::22b]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VdyLW-0007Gd-1d for linux-arm-kernel@lists.infradead.org; Wed, 06 Nov 2013 08:20:15 +0000 Received: by mail-lb0-f171.google.com with SMTP id x18so7468490lbi.16 for ; Wed, 06 Nov 2013 00:19:51 -0800 (PST) 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=3CnfQUSpoHb4f52utSza943wNPGkoTN/e9QvbLTQOvg=; b=Q80BDfGyGtYOLWDae/wmfRBTJi4Pq84wl0KhlS6r36lHVwe5oUTx6clcLkihNKoYpp XNiI4DcEC+NE8GAcqSBzTnugbUU/U6/posapsbGFs+i70AIr0ZjYfsLJ8oqk3v9HheNe +r4fOUjGXMqlb0eHORwnQy9BJmR6kzoJvilgPMWWlAuWukNe9ie5BBoYC/FcIYvuhu7s C2sutStpyF14CjlBizhKLbHlIq5fT8Rg/P4SMafDime7jGj0jou7nT+oDeBCjnHTK76t y+Cjjw++EUj5HmXhYlgrRig8RHpxX5El/R3fFi4xazndkcze6plSu4nG+af1VLclVQT6 ircQ== MIME-Version: 1.0 X-Received: by 10.152.5.69 with SMTP id q5mr47289laq.46.1383725988657; Wed, 06 Nov 2013 00:19:48 -0800 (PST) Received: by 10.114.25.136 with HTTP; Wed, 6 Nov 2013 00:19:48 -0800 (PST) In-Reply-To: <1410337.86GqGttdXc@avalon> References: <1383058511-26046-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1383058511-26046-4-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1410337.86GqGttdXc@avalon> Date: Wed, 6 Nov 2013 17:19:48 +0900 Message-ID: Subject: Re: [PATCH 3/3] clk: shmobile: Add R8A7790 clocks support From: Magnus Damm To: Laurent Pinchart X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20131106_032014_403347_7ECF9309 X-CRM114-Status: GOOD ( 52.96 ) X-Spam-Score: -2.0 (--) Cc: devicetree@vger.kernel.org, Laurent Pinchart , Mike Turquette , "linux-arm-kernel@lists.infradead.org" , SH-Linux X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable 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 Laurent, On Wed, Nov 6, 2013 at 8:47 AM, Laurent Pinchart wrote: > Hi Magnus, > > (And a question for Mike below) > > On Tuesday 05 November 2013 16:56:40 Magnus Damm wrote: >> On Tue, Oct 29, 2013 at 11:55 PM, Laurent Pinchart wrote: >> > The R8A7790 has several clocks that are too custom to be supported in a >> > generic driver. Those clocks can be divided in two categories: >> > >> > - Fixed rate clocks with multiplier and divisor set according to boot >> > mode configuration >> > >> > - Custom divider clocks with SoC-specific divider values >> > >> > This driver supports both. >> > >> > Signed-off-by: Laurent Pinchart >> > >> > --- >> > +void __init r8a7790_clocks_init(u32 mode) >> > +{ >> > + cpg_mode = mode; >> > + >> > + of_clk_init(NULL); >> > +} >> >> Hi Laurent, >> >> Thanks a lot for your efforts on this. In general I think it all looks good, > > Thank you. > >> I have however one question regarding the "probe" interface between the SoC >> code in arch/arm/mach-shmobile and drivers/clk. The code above in >> r8a7790_clocks_init() looks a bit spaghetti-style to me, perhaps it is >> possible to make it cleaner somehow? > > Isn't it just a loop ? :-) The clocks handled by this driver are "special", > they need to be initialized manually. Spaghetti-O's? =) I agree that some special handling is needed. >> I realize that you need some way to read out the mode pin setting, and that >> is currently provided by the SoC code in arch/arm/mach-shmobile/. Today it >> seems that you instead of letting the drivers/clk/ code call SoC code to get >> the mode pin setting (and add a SoC link dependency) you simply feed the >> settings to the clk code from the SoC code using the parameter to >> r8a7790_clocks_init(). This direction seems good to me. I'm however not too >> keen on the current symbol dependency in the "other" direction. >> >> If possible I'd like to keep the interface between the SoC code and the >> driver code to "standard" driver model functions. For instance, using >> of_clk_init(NULL) from the SoC code seems like a pretty good standard >> interface - without any link time dependencies. So with the current code, >> the r8a7790_clocks_init() function somehow goes against this no-symbol- >> dependency preference that I happen to have. =) >> >> Would it for instance be possible let the SoC code read out the mode pin >> setting, and then pass the current setting using the argument of >> of_clk_init() to select different dividers? That way the symbol dependency >> goes away. Or maybe it becomes too verbose? > > The of_clk_init() argument is an array of struct of_device_id that is used by > the clock core to match device tree nodes. I don't really see how you would > like to use it to pass the boot mode pins state to the driver. Yeah, it seems that interface isn't such a good match. Thanks. > There is currently no dedicated API (at least to my knowledge) to pass clock > configuration information between arch/arm and drivers/clk. Here's a couple of > methods that can be used. > > - Call a drivers/clk function from platform code with the clock configuration > information and store that information in a driver global variable for later > use (this is the method currently used by this patch). > > - Call a platform code function from driver code to read the configuration. > This is pretty similar to the above, with a dependency in the other direction. > > - Read the value directly from the hardware in the clock driver. This doesn't > feel really right, as that's not the job of the clock driver. In a more > general case this solution might not always be possible, as reading the > configuration might require access to resources not available to drivers. > > - Create a standard API for this purpose. It might be overengineering. > > - Use AUXDATA filled by platform code. > > There might be other solutions I haven't thought of. I went for the first > method as there's already 8 other clock drivers that expose an initialization > function to platform code. I might just have copied a mistake though. Thanks for listing these. I have come across another option: Use of_update_property() and of_property_read_u32() to pass using a property, see below. > Mike, do you have an opinion on this ? In a nutshell, the code clocks are > fixed factor but their factor depend on a boot mode configuration. The > configuration value is thus needed when instantiating the clocks. It can be > read from a hardware register, outside of the clocks IP core. Yeah, I too would be interested to hear what Mike thinks about this matter. Please see below for a prototype patch on top of your code that shows the driver side of my DT property proposal. The omitted SoC side becomes slightly more complicated but that code can be shared between multiple R-Car SoCs so I think it should be acceptable size-wise. With this patch applied the interface between the driver and the SoC code is a DT property (that needs documentation) and of_clk_init(NULL). No more evil link time symbol dependencies between arch/ and drivers/... drivers/clk/shmobile/clk-r8a7790.c | 15 ++++++--------- include/linux/clk/shmobile.h | 19 ------------------- 2 files changed, 6 insertions(+), 28 deletions(-) --- 0018/drivers/clk/shmobile/clk-r8a7790.c +++ work/drivers/clk/shmobile/clk-r8a7790.c 2013-11-06 16:59:59.000000000 +0 900 @@ -12,7 +12,6 @@ #include #include -#include #include #include #include @@ -70,8 +69,6 @@ static const struct clk_div_table cpg_sd { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, }; -static u32 cpg_mode __initdata; - #define CPG_SDCKCR 0x00000074 static void __init r8a7790_cpg_clocks_init(struct device_node *np) @ -80,6 +77,12 @@ static void __init r8a7790_cpg_clocks_in struct r8a7790_cpg *cpg; struct clk **clks; unsigned int i; + u32 cpg_mode; + + if (of_property_read_u32(node, "cpg-mode", &cpg_mode)) { + pr_err("%s: failed to determine CPG mode\n", __func__); + return; + } cpg = kzalloc(sizeof(*cpg), GFP_KERNEL); clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL); @ -168,9 +171,3 @@ static void __init r8a7790_cpg_clocks_in CLK_OF_DECLARE(r8a7790_cpg_clks, "renesas,r8a7790-cpg-clocks", r8a7790_cpg_clocks_init); -void __init r8a7790_clocks_init(u32 mode) -{ - cpg_mode = mode; - - of_clk_init(NULL); -} Cheers, / magnus --- 0018/include/linux/clk/shmobile.h +++ /dev/null 2013-06-03 21:41:10.638032047 +0900 @@ -1,19 +0,0 @@ -/* - * Copyright 2013 Ideas On Board SPRL - * - * Contact: Laurent Pinchart - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - */ - -#ifndef __LINUX_CLK_SHMOBILE_H_ -#define __LINUX_CLK_SHMOBILE_H_ - -#include - -void r8a7790_clocks_init(u32 mode); - -#endif