From patchwork Wed Apr 17 22:26:30 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Turquette X-Patchwork-Id: 2456841 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) by patchwork2.kernel.org (Postfix) with ESMTP id 0CBFEDF23A for ; Wed, 17 Apr 2013 22:26:48 +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 1USaoP-0003zD-UN; Wed, 17 Apr 2013 22:26:46 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1USaoN-000851-BT; Wed, 17 Apr 2013 22:26:43 +0000 Received: from mail-da0-x229.google.com ([2607:f8b0:400e:c00::229]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1USaoJ-00084Q-Dt for linux-arm-kernel@lists.infradead.org; Wed, 17 Apr 2013 22:26:40 +0000 Received: by mail-da0-f41.google.com with SMTP id p8so843505dan.0 for ; Wed, 17 Apr 2013 15:26:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:content-type:mime-version:content-transfer-encoding:to :from:in-reply-to:cc:references:message-id:user-agent:subject:date :x-gm-message-state; bh=Y8JETkQ4bqTAs7uci0ro2FL/YaNvVQ7TJNc2kvf3uPo=; b=cAEboISFrcdcyruX+d4FiL4yD3/THyfiSLtjvcYSKPS2Ubl2iyuLEOjxFIyigiBzgJ /xloWKMiDYXMuYFeznRGyyXHx3GEmgauq9wDTiEDWBq6EGJr63nou58mud4zzgq83yYc z6jjNuRHNU1dH0Pc19cp05dp6SAo5jyzfi44ELyGYrWFA7xY6zp9/GvbXkpSVcQqZA8Y TKkniweJDq4BgVo88D+nPAiUYTMtwO1xa3V8Jhc7yqrvCNkrRhG8mLAMtkcMXJKWk27t thXX7BDN/dAXiwmbCSc8DH++lhwjevuw3O1CVU27h37MaOCASj5A1gKM/66sRJiU+vS3 d3YQ== X-Received: by 10.68.160.226 with SMTP id xn2mr10915259pbb.174.1366237596462; Wed, 17 Apr 2013 15:26:36 -0700 (PDT) Received: from localhost (adsl-69-228-93-79.dsl.pltn13.pacbell.net. [69.228.93.79]) by mx.google.com with ESMTPS id fq1sm7489915pbb.33.2013.04.17.15.26.33 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 17 Apr 2013 15:26:35 -0700 (PDT) MIME-Version: 1.0 To: Pawel Moll , From: Mike Turquette In-Reply-To: <1363358409-5693-1-git-send-email-pawel.moll@arm.com> References: <1363358409-5693-1-git-send-email-pawel.moll@arm.com> Message-ID: <20130417222630.19887.75481@quantum> User-Agent: alot/0.3.4 Subject: Re: [PATCH] clk: vexpress: Add separate SP810 driver Date: Wed, 17 Apr 2013 15:26:30 -0700 X-Gm-Message-State: ALoCoQm52c7QeDAXC3XW8GbkVOOSMX/LRvXyeLK4DVWJdCLGKObUAQZMe9axPkrspY6o2nRZ244z X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130417_182639_636046_8D0E469D X-CRM114-Status: GOOD ( 28.70 ) X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-1.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Catalin Marinas , Pawel Moll , linux-arm-kernel@lists.infradead.org, Rob Herring 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 Quoting Pawel Moll (2013-03-15 07:40:09) > +static int clk_sp810_timerclken_prepare(struct clk_hw *hw) > +{ > + struct clk_sp810_timerclken *timerclken = to_clk_sp810_timerclken(hw); > + struct clk_sp810 *sp810 = timerclken->sp810; > + struct clk *refclk = of_clk_get(sp810->node, sp810->refclk_index); > + struct clk *timclk = of_clk_get(sp810->node, sp810->timclk_index); > + struct clk *old_parent = __clk_get_parent(hw->clk); > + struct clk *new_parent = old_parent; > + int new_parent_index; > + > + if (WARN_ON(IS_ERR(refclk) || IS_ERR(timclk))) > + return -ENOENT; > + > + /* Select "better" (faster) parent */ > + if (__clk_get_rate(refclk) > __clk_get_rate(timclk)) { > + new_parent = refclk; > + new_parent_index = 0; > + } else { > + new_parent = timclk; > + new_parent_index = 1; > + } > + > + /* Switch the parent if necessary */ > + if (old_parent != new_parent) { > + __clk_prepare(new_parent); > + clk_sp810_timerclken_set_parent(hw, new_parent_index); > + __clk_reparent(hw->clk, new_parent); > + __clk_unprepare(old_parent); > + } The reentrancy patch makes some of this less messy. I have a re-worked patch below demonstrating this. > + > + return 0; > +} > + > +static const struct clk_ops clk_sp810_timerclken_ops = { > + .prepare = clk_sp810_timerclken_prepare, > + .get_parent = clk_sp810_timerclken_get_parent, > + .set_parent = clk_sp810_timerclken_set_parent, > +}; I dislike these one-off clk_prepare hacks. Any time I see a .prepare callback and not a matching .unprepare callback I know something is wrong. At a minimum you should be calling clk_put for the clocks you "get" from of_clk_get. In this case you are using clk_prepare for one-time configuration. I agree that no alternative exists yet, but there have been discussions recently about using DT for configuration details that are not strictly descriptions of the hardware. I've added a comment block in the patch below which notes this with a FIXME since there isn't a graceful alternative to using clk_prepare for ont-time configuration. Can you test the patch below against the latest clk-next and let me know if it working on your platform? Regards, Mike From e3f11f34d4f7d3c6c89c43c2100f246d3b91e903 Mon Sep 17 00:00:00 2001 From: Pawel Moll Date: Fri, 15 Mar 2013 14:40:09 +0000 Subject: [PATCH] clk: vexpress: Add separate SP810 driver Factor out the SP810 clocking code into a separate driver, selecting better (faster) parent at clk_prepare() time. This is to avoid problems with clocking infrastructure initialisation order, in particular to avoid dependency of fixed clock being initialized before SP810. It also makes vexpress platform OF-based clock initialisation code unnecessary. Signed-off-by: Pawel Moll Tested-by: Catalin Marinas Signed-off-by: Mike Turquette [mturquette@linaro.org: add .unprepare, FIXME comment, cleaned up code] --- arch/arm/mach-vexpress/v2m.c | 8 +- drivers/clk/versatile/Makefile | 2 +- drivers/clk/versatile/clk-sp810.c | 186 ++++++++++++++++++++++++++++++++++ drivers/clk/versatile/clk-vexpress.c | 49 --------- 4 files changed, 194 insertions(+), 51 deletions(-) create mode 100644 drivers/clk/versatile/clk-sp810.c diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c index 915683c..c5e20b5 100644 --- a/arch/arm/mach-vexpress/v2m.c +++ b/arch/arm/mach-vexpress/v2m.c @@ -21,6 +21,8 @@ #include #include #include +#include +#include #include #include @@ -433,7 +435,7 @@ static void __init v2m_dt_timer_init(void) { struct device_node *node = NULL; - vexpress_clk_of_init(); + of_clk_init(NULL); do { node = of_find_compatible_node(node, NULL, "arm,sp804"); @@ -441,6 +443,10 @@ static void __init v2m_dt_timer_init(void) if (node) { pr_info("Using SP804 '%s' as a clock & events source\n", node->full_name); + WARN_ON(clk_register_clkdev(of_clk_get_by_name(node, + "timclken1"), "v2m-timer0", "sp804")); + WARN_ON(clk_register_clkdev(of_clk_get_by_name(node, + "timclken2"), "v2m-timer1", "sp804")); v2m_sp804_init(of_iomap(node, 0), irq_of_parse_and_map(node, 0)); } diff --git a/drivers/clk/versatile/Makefile b/drivers/clk/versatile/Makefile index ec3b88f..c16ca78 100644 --- a/drivers/clk/versatile/Makefile +++ b/drivers/clk/versatile/Makefile @@ -3,5 +3,5 @@ obj-$(CONFIG_ICST) += clk-icst.o obj-$(CONFIG_ARCH_INTEGRATOR) += clk-integrator.o obj-$(CONFIG_INTEGRATOR_IMPD1) += clk-impd1.o obj-$(CONFIG_ARCH_REALVIEW) += clk-realview.o -obj-$(CONFIG_ARCH_VEXPRESS) += clk-vexpress.o +obj-$(CONFIG_ARCH_VEXPRESS) += clk-vexpress.o clk-sp810.o obj-$(CONFIG_VEXPRESS_CONFIG) += clk-vexpress-osc.o diff --git a/drivers/clk/versatile/clk-sp810.c b/drivers/clk/versatile/clk-sp810.c new file mode 100644 index 0000000..295375a --- /dev/null +++ b/drivers/clk/versatile/clk-sp810.c @@ -0,0 +1,186 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Copyright (C) 2013 ARM Limited + */ + +#include +#include +#include +#include +#include +#include + +#define to_clk_sp810_timerclken(_hw) \ + container_of(_hw, struct clk_sp810_timerclken, hw) + +struct clk_sp810; + +struct clk_sp810_timerclken { + struct clk_hw hw; + struct clk *clk; + struct clk_sp810 *sp810; + int channel; +}; + +struct clk_sp810 { + struct device_node *node; + int refclk_index, timclk_index; + void __iomem *base; + spinlock_t lock; + struct clk_sp810_timerclken timerclken[4]; + struct clk *refclk; + struct clk *timclk; +}; + +static u8 clk_sp810_timerclken_get_parent(struct clk_hw *hw) +{ + struct clk_sp810_timerclken *timerclken = to_clk_sp810_timerclken(hw); + u32 val = readl(timerclken->sp810->base + SCCTRL); + + return !!(val & (1 << SCCTRL_TIMERENnSEL_SHIFT(timerclken->channel))); +} + +static int clk_sp810_timerclken_set_parent(struct clk_hw *hw, u8 index) +{ + struct clk_sp810_timerclken *timerclken = to_clk_sp810_timerclken(hw); + struct clk_sp810 *sp810 = timerclken->sp810; + u32 val, shift = SCCTRL_TIMERENnSEL_SHIFT(timerclken->channel); + unsigned long flags = 0; + + if (WARN_ON(index > 1)) + return -EINVAL; + + spin_lock_irqsave(&sp810->lock, flags); + + val = readl(sp810->base + SCCTRL); + val &= ~(1 << shift); + val |= index << shift; + writel(val, sp810->base + SCCTRL); + + spin_unlock_irqrestore(&sp810->lock, flags); + + return 0; +} + +/* + * FIXME - setting the parent every time .prepare is invoked is inefficient. + * This is better handled by a dedicated clock tree configuration mechanism at + * init-time. Revisit this later when such a mechanism exists + */ +static int clk_sp810_timerclken_prepare(struct clk_hw *hw) +{ + struct clk_sp810_timerclken *timerclken = to_clk_sp810_timerclken(hw); + struct clk_sp810 *sp810 = timerclken->sp810; + struct clk *old_parent = __clk_get_parent(hw->clk); + struct clk *new_parent; + + if (!sp810->refclk) + sp810->refclk = of_clk_get(sp810->node, sp810->refclk_index); + + if (!sp810->timclk) + sp810->timclk = of_clk_get(sp810->node, sp810->timclk_index); + + if (WARN_ON(IS_ERR(sp810->refclk) || IS_ERR(sp810->timclk))) + return -ENOENT; + + /* Select fastest parent */ + if (clk_get_rate(sp810->refclk) > clk_get_rate(sp810->timclk)) { + new_parent = sp810->refclk; + } else { + new_parent = sp810->timclk; + } + + /* Switch the parent if necessary */ + if (old_parent != new_parent) + clk_set_parent(hw->clk, new_parent); + + return 0; +} + +static void clk_sp810_timerclken_unprepare(struct clk_hw *hw) +{ + struct clk_sp810_timerclken *timerclken = to_clk_sp810_timerclken(hw); + struct clk_sp810 *sp810 = timerclken->sp810; + + clk_put(sp810->timclk); + clk_put(sp810->refclk); +} + +static const struct clk_ops clk_sp810_timerclken_ops = { + .prepare = clk_sp810_timerclken_prepare, + .unprepare = clk_sp810_timerclken_unprepare, + .get_parent = clk_sp810_timerclken_get_parent, + .set_parent = clk_sp810_timerclken_set_parent, +}; + +struct clk *clk_sp810_timerclken_of_get(struct of_phandle_args *clkspec, + void *data) +{ + struct clk_sp810 *sp810 = data; + + if (WARN_ON(clkspec->args_count != 1 || clkspec->args[0] > + ARRAY_SIZE(sp810->timerclken))) + return NULL; + + return sp810->timerclken[clkspec->args[0]].clk; +} + +void __init clk_sp810_of_setup(struct device_node *node) +{ + struct clk_sp810 *sp810 = kzalloc(sizeof(*sp810), GFP_KERNEL); + const char *parent_names[2]; + char name[12]; + struct clk_init_data init; + int i; + + if (!sp810) { + pr_err("Failed to allocate memory for SP810!\n"); + return; + } + + sp810->refclk_index = of_property_match_string(node, "clock-names", + "refclk"); + parent_names[0] = of_clk_get_parent_name(node, sp810->refclk_index); + + sp810->timclk_index = of_property_match_string(node, "clock-names", + "timclk"); + parent_names[1] = of_clk_get_parent_name(node, sp810->timclk_index); + + if (parent_names[0] <= 0 || parent_names[1] <= 0) { + pr_warn("Failed to obtain parent clocks for SP810!\n"); + return; + } + + sp810->node = node; + sp810->base = of_iomap(node, 0); + spin_lock_init(&sp810->lock); + + init.name = name; + init.ops = &clk_sp810_timerclken_ops; + init.flags = CLK_IS_BASIC; + init.parent_names = parent_names; + init.num_parents = ARRAY_SIZE(parent_names); + + for (i = 0; i < ARRAY_SIZE(sp810->timerclken); i++) { + snprintf(name, ARRAY_SIZE(name), "timerclken%d", i); + + sp810->timerclken[i].sp810 = sp810; + sp810->timerclken[i].channel = i; + sp810->timerclken[i].hw.init = &init; + + sp810->timerclken[i].clk = clk_register(NULL, + &sp810->timerclken[i].hw); + WARN_ON(IS_ERR(sp810->timerclken[i].clk)); + } + + of_clk_add_provider(node, clk_sp810_timerclken_of_get, sp810); +} +CLK_OF_DECLARE(sp810, "arm,sp810", clk_sp810_of_setup); diff --git a/drivers/clk/versatile/clk-vexpress.c b/drivers/clk/versatile/clk-vexpress.c index 82b45aa..a4a728d 100644 --- a/drivers/clk/versatile/clk-vexpress.c +++ b/drivers/clk/versatile/clk-vexpress.c @@ -15,8 +15,6 @@ #include #include #include -#include -#include #include static struct clk *vexpress_sp810_timerclken[4]; @@ -86,50 +84,3 @@ void __init vexpress_clk_init(void __iomem *sp810_base) WARN_ON(clk_register_clkdev(vexpress_sp810_timerclken[1], "v2m-timer1", "sp804")); } - -#if defined(CONFIG_OF) - -struct clk *vexpress_sp810_of_get(struct of_phandle_args *clkspec, void *data) -{ - if (WARN_ON(clkspec->args_count != 1 || clkspec->args[0] > - ARRAY_SIZE(vexpress_sp810_timerclken))) - return NULL; - - return vexpress_sp810_timerclken[clkspec->args[0]]; -} - -void __init vexpress_clk_of_init(void) -{ - struct device_node *node; - struct clk *clk; - struct clk *refclk, *timclk; - - of_clk_init(NULL); - - node = of_find_compatible_node(NULL, NULL, "arm,sp810"); - vexpress_sp810_init(of_iomap(node, 0)); - of_clk_add_provider(node, vexpress_sp810_of_get, NULL); - - /* Select "better" (faster) parent for SP804 timers */ - refclk = of_clk_get_by_name(node, "refclk"); - timclk = of_clk_get_by_name(node, "timclk"); - if (!WARN_ON(IS_ERR(refclk) || IS_ERR(timclk))) { - int i = 0; - - if (clk_get_rate(refclk) > clk_get_rate(timclk)) - clk = refclk; - else - clk = timclk; - - for (i = 0; i < ARRAY_SIZE(vexpress_sp810_timerclken); i++) - WARN_ON(clk_set_parent(vexpress_sp810_timerclken[i], - clk)); - } - - WARN_ON(clk_register_clkdev(vexpress_sp810_timerclken[0], - "v2m-timer0", "sp804")); - WARN_ON(clk_register_clkdev(vexpress_sp810_timerclken[1], - "v2m-timer1", "sp804")); -} - -#endif