From patchwork Thu Apr 18 17:23:22 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pawel Moll X-Patchwork-Id: 2461941 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 6AA1DDF2E5 for ; Thu, 18 Apr 2013 17:23:41 +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 1USsYc-00013S-Qk; Thu, 18 Apr 2013 17:23:39 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1USsYZ-0004YK-TX; Thu, 18 Apr 2013 17:23:35 +0000 Received: from service87.mimecast.com ([91.220.42.44]) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1USsYV-0004Xj-11 for linux-arm-kernel@lists.infradead.org; Thu, 18 Apr 2013 17:23:33 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Thu, 18 Apr 2013 18:23:24 +0100 Received: from [10.2.201.40] ([10.1.255.212]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Thu, 18 Apr 2013 18:23:22 +0100 Message-ID: <1366305802.3077.37.camel@hornet> Subject: Re: [PATCH] clk: vexpress: Add separate SP810 driver From: Pawel Moll To: Mike Turquette Date: Thu, 18 Apr 2013 18:23:22 +0100 In-Reply-To: <20130417222630.19887.75481@quantum> References: <1363358409-5693-1-git-send-email-pawel.moll@arm.com> <20130417222630.19887.75481@quantum> X-Mailer: Evolution 3.6.2-0ubuntu0.1 Mime-Version: 1.0 X-OriginalArrivalTime: 18 Apr 2013 17:23:22.0573 (UTC) FILETIME=[6D69CFD0:01CE3C59] X-MC-Unique: 113041818232418401 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130418_132331_402783_FC9EF66A X-CRM114-Status: GOOD ( 27.49 ) X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [91.220.42.44 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Catalin Marinas , "linux-arm-kernel@lists.infradead.org" , "rob.herring@calxeda.com" 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 On Wed, 2013-04-17 at 23:26 +0100, Mike Turquette wrote: > 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. Fair enough. > Can you test the patch below against the latest clk-next and let me know > if it working on your platform? It wasn't: WARNING: at [...]/drivers/clk/clk.c:808 __clk_enable+0x94/0xa0() because the new parent is not prepared automagically. This simple change heals the problem and now everything is just fine: /* Switch the parent if necessary */ - if (old_parent != new_parent) + if (old_parent != new_parent) { + clk_prepare(new_parent); clk_set_parent(hw->clk, new_parent); + clk_unprepare(old_parent); + } So, to summarize, the patch that works for me is below. Thanks for you help! Pawel 8<-------------------------------- From 58807cc1930edd092f1bc54b2ec381ddc9209671 Mon Sep 17 00:00:00 2001 From: Pawel Moll Date: Thu, 18 Apr 2013 18:20:54 +0100 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 | 188 ++++++++++++++++++++++++++++++++++ drivers/clk/versatile/clk-vexpress.c | 49 --------- 4 files changed, 196 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..bf9b15a --- /dev/null +++ b/drivers/clk/versatile/clk-sp810.c @@ -0,0 +1,188 @@ +/* + * 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_prepare(new_parent); + clk_set_parent(hw->clk, new_parent); + clk_unprepare(old_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