Re: [PATCH v3 05/19] clk: at91: add PMC main clock

[boris brezillon <b.brezillon@overkiz.com>]

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@overkiz.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@overkiz.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.

> 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).

>
>> +
>> +    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_ */
>>
>
>