| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5253C9C1.9020402@overkiz.com>
Date: Tue, 08 Oct 2013 11:00:49 +0200
From: boris brezillon <b.brezillon@...rkiz.com>
To: Nicolas Ferre <nicolas.ferre@...el.com>,
Grant Likely <grant.likely@...aro.org>,
Rob Herring <rob.herring@...xeda.com>,
Rob Landley <rob@...dley.net>,
Andrew Victor <linux@...im.org.za>,
Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
Russell King <linux@....linux.org.uk>,
Mike Turquette <mturquette@...aro.org>,
Felipe Balbi <balbi@...com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Ludovic Desroches <ludovic.desroches@...el.com>,
Josh Wu <josh.wu@...el.com>,
Richard Genoud <richard.genoud@...il.com>
CC: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 05/19] clk: at91: add PMC main clock
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 <b.brezillon@...rkiz.com>
>>>> ---
>>>> 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 <b.brezillon@...rkiz.com>
>>>> + *
>>>> + * 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 <linux/clk-provider.h>
>>>> +#include <linux/clkdev.h>
>>>> +#include <linux/clk/at91_pmc.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/irq.h>
>>>> +#include <linux/sched.h>
>>>> +#include <linux/wait.h>
>>>> +
>>>> +#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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists