Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753771Ab3JHJCJ (ORCPT ); Tue, 8 Oct 2013 05:02:09 -0400 Received: from 11.mo4.mail-out.ovh.net ([46.105.34.195]:41121 "EHLO mo4.mail-out.ovh.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752028Ab3JHJCE (ORCPT ); Tue, 8 Oct 2013 05:02:04 -0400 Message-ID: <5253C9C1.9020402@overkiz.com> Date: Tue, 08 Oct 2013 11:00:49 +0200 From: boris brezillon User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.0 MIME-Version: 1.0 To: Nicolas Ferre , Grant Likely , Rob Herring , Rob Landley , Andrew Victor , Jean-Christophe Plagniol-Villard , Russell King , Mike Turquette , Felipe Balbi , Greg Kroah-Hartman , Ludovic Desroches , Josh Wu , Richard Genoud CC: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org X-Ovh-Mailout: 178.32.228.4 (mo4.mail-out.ovh.net) Subject: Re: [PATCH v3 05/19] clk: at91: add PMC main clock References: <1375937608-3773-1-git-send-email-b.brezillon@overkiz.com> <1375938369-4002-1-git-send-email-b.brezillon@overkiz.com> <5252E6AB.1000607@atmel.com> <52530744.60701@overkiz.com> <5253C142.2080808@atmel.com> In-Reply-To: <5253C142.2080808@atmel.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Ovh-Tracer-Id: 14543249097604364396 X-Ovh-Remote: 80.245.18.66 () X-Ovh-Local: 213.186.33.20 (ns0.ovh.net) X-OVH-SPAMSTATE: OK X-OVH-SPAMSCORE: -100 X-OVH-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrfeeiledrvddtucetufdoteggodetrfcurfhrohhfihhlvgemucfqggfjnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd X-Spam-Check: DONE|U 0.5/N X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrfeeiledrvddtucetufdoteggodetrfcurfhrohhfihhlvgemucfqggfjnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10224 Lines: 317 On 08/10/2013 10:24, Nicolas Ferre wrote: > On 07/10/2013 21:11, boris brezillon : >> On 07/10/2013 18:51, Nicolas Ferre wrote: >>> On 08/08/2013 07:06, Boris BREZILLON : >>>> This patch adds new at91 main oscillator clock implementation using >>>> common >>>> clk framework. >>>> >>>> If rate is not provided during clock registration it is calculated >>>> using >>>> the slow clock (main clk parent in this case) rate and MCFR register. >>>> >>>> Signed-off-by: Boris BREZILLON >>>> --- >>>> drivers/clk/at91/Makefile | 1 + >>>> drivers/clk/at91/clk-main.c | 171 >>>> +++++++++++++++++++++++++++++++++++++++++++ >>>> drivers/clk/at91/pmc.c | 5 ++ >>>> drivers/clk/at91/pmc.h | 3 + >>>> 4 files changed, 180 insertions(+) >>>> create mode 100644 drivers/clk/at91/clk-main.c >>>> >>>> diff --git a/drivers/clk/at91/Makefile b/drivers/clk/at91/Makefile >>>> index 1d4fb21..44105bd 100644 >>>> --- a/drivers/clk/at91/Makefile >>>> +++ b/drivers/clk/at91/Makefile >>>> @@ -3,3 +3,4 @@ >>>> # >>>> >>>> obj-y += pmc.o >>>> +obj-y += clk-main.o >>>> diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c >>>> new file mode 100644 >>>> index 0000000..68738dd >>>> --- /dev/null >>>> +++ b/drivers/clk/at91/clk-main.c >>>> @@ -0,0 +1,171 @@ >>>> +/* >>>> + * drivers/clk/at91/clk-main.c >>>> + * >>>> + * Copyright (C) 2013 Boris BREZILLON >>>> + * >>>> + * 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. >>>> + * >>>> + */ >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#include "pmc.h" >>>> + >>>> +#define to_clk_main(hw) container_of(hw, struct clk_main, hw) >>>> +struct clk_main { >>>> + struct clk_hw hw; >>>> + struct at91_pmc *pmc; >>>> + unsigned long rate; >>>> + unsigned int irq; >>>> + wait_queue_head_t wait; >>>> +}; >>>> + >>>> +static irqreturn_t clk_main_irq_handler(int irq, void *dev_id) >>>> +{ >>>> + struct clk_main *clkmain = (struct clk_main *)dev_id; >>>> + >>>> + wake_up(&clkmain->wait); >>>> + disable_irq_nosync(clkmain->irq); >>>> + >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static int clk_main_prepare(struct clk_hw *hw) >>>> +{ >>>> + struct clk_main *clkmain = to_clk_main(hw); >>>> + struct at91_pmc *pmc = clkmain->pmc; >>>> + >>>> + while (!(pmc_read(pmc, AT91_PMC_SR) & AT91_PMC_MOSCS)) { >>>> + enable_irq(clkmain->irq); >>>> + wait_event(clkmain->wait, >>>> + pmc_read(pmc, AT91_PMC_SR) & AT91_PMC_MOSCS); >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int clk_main_is_prepared(struct clk_hw *hw) >>>> +{ >>>> + struct clk_main *clkmain = to_clk_main(hw); >>>> + >>>> + return !!(pmc_read(clkmain->pmc, AT91_PMC_SR) & AT91_PMC_MOSCS); >>>> +} >>>> + >>>> +static unsigned long clk_main_recalc_rate(struct clk_hw *hw, >>>> + unsigned long parent_rate) >>>> +{ >>>> + u32 tmp; >>>> + struct clk_main *clkmain = to_clk_main(hw); >>>> + struct at91_pmc *pmc = clkmain->pmc; >>>> + >>>> + if (clkmain->rate) >>>> + return clkmain->rate; >>>> + >>>> + while (!((tmp = pmc_read(pmc, AT91_CKGR_MCFR)) & >>>> AT91_PMC_MAINRDY)) >>>> + ; >>> >>> I must say that I do not like this while() loop. Please implement a >>> way out and a mean to release CPU when waiting... >>> >>> Maybe taking a timeout example like this one: >>> http://lxr.free-electrons.com/source/drivers/net/ethernet/cadence/macb.c#L391 >>> >>> >>> >>> Which is actually what I leart from Wolfram during his interesting >>> session @ ELC-E: >>> http://elinux.org/images/5/54/Elce11_sang.pdf >>> >> >> I'll take a closer look at these references. >> As I understand, this is about sleeping between each test/iteration of >> the while loop. >> Mike, I'm not sure if the recalc_rate callback can actually sleep. If this is not the case, we should move this waiting loop in clk_prepare. What do you think ? >>> It is true though that we should keep this very quick to not alter >>> boot time too much. >>> >>> I also wonder if really checking that main clock is read makes sense. >>> If we are running Linux it probably mean that at least main clock is >>> already well configured by the bootloader. >> >> This is not exactly the status of the main clk (the main clk status is >> reflected by the MOSCXTS bit in the PMC_SR register). >> This field reflects the MAINF value reliability (MAINF value is used in >> case the main XTAL frequency was not provided in the dt definition, >> to calculate the main XTAL frequency). >> Anyway, MAINFRDY will probably be set at the time this code is executed. >> >>> >>> And this lead to a more general remark: couldn't we simply state that >>> this clock is fixed and only available for reading its status? >>> >> This is up to you, but IMHO the code to support this (unlikely) case is >> not so big (even with the sleeping version you suggested). > > Fair enough, let's go for this version. > >>>> + >>>> + tmp &= AT91_PMC_MAINF; >>>> + clkmain->rate = (tmp * parent_rate) / 16; >>>> + >>>> + return clkmain->rate; >>>> +} >>>> + >>>> +static const struct clk_ops main_ops = { >>>> + .prepare = clk_main_prepare, >>>> + .is_prepared = clk_main_is_prepared, >>>> + .recalc_rate = clk_main_recalc_rate, >>>> +}; >>>> + >>>> +static struct clk * __init >>>> +at91_clk_register_main(struct at91_pmc *pmc, >>>> + unsigned int irq, >>>> + const char *name, >>>> + const char *parent_name, >>>> + unsigned long rate) >>>> +{ >>>> + int ret; >>>> + struct clk_main *clkmain; >>>> + struct clk *clk = NULL; >>>> + struct clk_init_data init; >>>> + >>>> + if (!pmc || !irq || !name) >>>> + return ERR_PTR(-EINVAL); >>>> + >>>> + if (!rate && !parent_name) >>>> + return ERR_PTR(-EINVAL); >>>> + >>>> + clkmain = kzalloc(sizeof(*clkmain), GFP_KERNEL); >>>> + if (!clkmain) >>>> + return ERR_PTR(-ENOMEM); >>>> + >>>> + init.name = name; >>>> + init.ops = &main_ops; >>>> + init.parent_names = parent_name ? &parent_name : NULL; >>>> + init.num_parents = parent_name ? 1 : 0; >>>> + init.flags = parent_name ? 0 : CLK_IS_ROOT; >>>> + >>>> + clkmain->hw.init = &init; >>>> + clkmain->rate = rate; >>>> + clkmain->pmc = pmc; >>>> + clkmain->irq = irq; >>>> + init_waitqueue_head(&clkmain->wait); >>>> + irq_set_status_flags(clkmain->irq, IRQ_NOAUTOEN); >>>> + ret = request_irq(clkmain->irq, clk_main_irq_handler, >>>> + IRQF_TRIGGER_HIGH, "clk-main", clkmain); >>>> + if (ret) >>>> + return ERR_PTR(ret); >>>> + >>>> + clk = clk_register(NULL, &clkmain->hw); >>>> + >>>> + if (IS_ERR(clk)) { >>>> + free_irq(clkmain->irq, clkmain); >>>> + kfree(clkmain); >>>> + } >>>> + >>>> + return clk; >>>> +} >>>> + >>>> + >>>> + >>>> +static void __init >>>> +of_at91_clk_main_setup(struct device_node *np, struct at91_pmc *pmc) >>>> +{ >>>> + struct clk *clk; >>>> + unsigned int irq; >>>> + const char *parent_name; >>>> + const char *name = np->name; >>>> + u32 rate = 0; >>>> + >>>> + parent_name = of_clk_get_parent_name(np, 0); >>>> + of_property_read_string(np, "clock-output-names", &name); >>>> + of_property_read_u32(np, "clock-frequency", &rate); >>>> + irq = irq_of_parse_and_map(np, 0); >>>> + if (!irq) >>>> + return; >>>> + >>>> + clk = at91_clk_register_main(pmc, irq, name, parent_name, rate); >>>> + >>>> + if (!IS_ERR(clk)) >>>> + return; >>>> + >>>> + of_clk_add_provider(np, of_clk_src_simple_get, clk); >>> >>> I do not understand this part: we add the provider if we have not >>> registered the clock correctly? >> This is a bug. :) >> >> I haven't noticed it, because we do not directly request this clk using >> of_clk_get or clk_get function. >> Instead this clk is used as a parent for several clks (plls, master, >> ...) and in this case the common clk framework only >> use parent clk names, not of clk provider to retrieve the appropriate >> clk. >> >> Anyway, thanks for catching this bug, I'll fix it. >> >>> >>> >>>> +} >>>> + >>>> +void __init of_at91rm9200_clk_main_setup(struct device_node *np, >>>> + struct at91_pmc *pmc) >>>> +{ >>>> + of_at91_clk_main_setup(np, pmc); >>>> +} >>>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c >>>> index f6bd03d..42f0c19 100644 >>>> --- a/drivers/clk/at91/pmc.c >>>> +++ b/drivers/clk/at91/pmc.c >>>> @@ -216,6 +216,11 @@ out_free_pmc: >>>> } >>>> >>>> static const struct of_device_id pmc_clk_ids[] __initdata = { >>>> + /* Main clock */ >>>> + { >>>> + .compatible = "atmel,at91rm9200-clk-main", >>>> + .data = of_at91rm9200_clk_main_setup >>>> + }, >>>> { /*sentinel*/ } >>>> }; >>>> >>>> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h >>>> index b7e8397..294e230 100644 >>>> --- a/drivers/clk/at91/pmc.h >>>> +++ b/drivers/clk/at91/pmc.h >>>> @@ -55,4 +55,7 @@ static inline void pmc_write(struct at91_pmc *pmc, >>>> int offset, u32 value) >>>> return writel_relaxed(value, pmc->regbase + offset); >>>> } >>>> >>>> +extern void __init of_at91rm9200_clk_main_setup(struct device_node >>>> *np, >>>> + struct at91_pmc *pmc); >>>> + >>>> #endif /* __PMC_H_ */ >>>> >>> >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/