Re: [PATCH] clk: Add device tree binding to clk-fixed-factor

From: Gregory CLEMENT
Date: Fri Apr 12 2013 - 05:05:09 EST

On 04/12/2013 08:54 AM, Christian Ruppert wrote:
> On Thu, Apr 11, 2013 at 06:26:07PM +0200, Gregory CLEMENT wrote:
>> On 04/11/2013 11:19 AM, Christian Ruppert wrote:
>>> Hi Gregory,
>>>
>>> Since there doesn't seem to be anyone opposing this feature I just
>>> checked your patch and after adding the CLK_OF_DECLARE macro in the end
>>> it works well for us. Tell me if you would like to rebase and resubmit
>>> your patch or if I should follow up on our own.
>>
>> I can rebase and resubmit my patch, it's pretty straightforward.
>> The problem was the lack of user in the kernel. And by user I mean
>> a driver using these functions. Usually a new function enter in the
>> kernel only if there are users for it inside the kernel so if you plan
>> to submit a driver using this piece of code, then I see no reason
>> to not get it.
>
> If you add the line
>
> CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clkdiv",

Why did you add the div suffix?

There is nothing in the function which prevent to have "mul" greater than
"div" and hence having a multiplicative factor.

I think that this suffix is misleading.

>
> in the end of drivers/clk/clk-fixed-factor.c, the new function will be
> automatically used by of_clk_init(NULL) which itself is used in numerous
> platform ports, in particular arch/arc/plat-tb10x. plat-tb10x is currently
> being integrated in the ARC architecture tree. The device tree file
> requiring this patch is already in linux-next (see
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arc/boot/dts/abilis_*.dtsi).
>
> We didn't know if you wanted to resubmit your patch or if I should
> pursue mine at the time, and today it uses the naming scheme from mine.
> It would be nice if you could either adapt your patch to that naming
> scheme.
>
> Greetings,
> Christian
>
>>> Greetings,
>>> Christian
>>>
>>> On Wed, Apr 10, 2013 at 06:27:47PM +0200, christian ruppert wrote:
>>>> Hi Gregory,
>>>>
>>>> We use derived clocks with fixed ratios for our TB10x platform. I
>>>> originally had this in the platform code but Vineet (the platform
>>>> maintainer) rightly pointed out that the common clock framework would
>>>> be the correct place for it.
>>>> Is there any reason not to support this functionality? After all, the
>>>> generic fixed rate clock also has a device tree binding and I'd guess
>>>> we're not the only ones statically deriving one clock from another in
>>>> our SOCs.
>>>>
>>>> Greetings,
>>>> Christian
>>>>
>>>> On Wed, Apr 10, 2013 at 05:56:25PM +0200, Gregory CLEMENT wrote:
>>>>> Hi Christian,
>>>>>
>>>>> On 04/10/2013 05:40 PM, Christian Ruppert wrote:
>>>>>> This patch adds a device tree binding for the simple fixed factor clock
>>>>>> divider/multiplier of the common clock tree binding.
>>>>>
>>>>> This patch remind me of something :
>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-August/116405.html
>>>>>
>>>>> Do you have a use case for it, this time?
>>>>>
>>>>> My version have been hold off waiting for an user for it.
>>>>> Will you need it for a driver?
>>>>>
>>>>> Regards,
>>>>>
>>>>>>
>>>>>> Signed-off-by: Christian Ruppert <christian.ruppert@xxxxxxxxxx>
>>>>>> ---
>>>>>> .../bindings/clock/fixed-factor-clkdiv.txt | 24 +++++++++++++++
>>>>>> drivers/clk/clk-fixed-factor.c | 32 ++++++++++++++++++++
>>>>>> include/linux/clk-provider.h | 1 +
>>>>>> 3 files changed, 57 insertions(+), 0 deletions(-)
>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/fixed-factor-clkdiv.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/clock/fixed-factor-clkdiv.txt b/Documentation/devicetree/bindings/clock/fixed-factor-clkdiv.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..352bac4
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/clock/fixed-factor-clkdiv.txt
>>>>>> @@ -0,0 +1,24 @@
>>>>>> +Device Tree Clock bindings for plat-tb10x
>>>>>> +
>>>>>> +This binding uses the common clock binding[1].
>>>>>> +
>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible : shall be "fixed-factor-clkdiv"
>>>>>> +- #clock-cells: from common clock binding; shall be set to 0
>>>>>> +- clocks: shall be the input parent clock phandle for the clock.
>>>>>> +- clock-mult: defines the multiplication factor of the output clock frequency
>>>>>> + wrt. the input clock frequency.
>>>>>> +- clock-div: defines the division factor of the output clock frequency wrt.
>>>>>> + the input clock frequency.
>>>>>> +
>>>>>> +Example:
>>>>>> +cpu_clk: clkdiv_cpu { /* CPU clock derived from pll0. 1/2 of pll frequency */
>>>>>> + compatible = "fixed-factor-clkdiv";
>>>>>> + #clock-cells = <0>;
>>>>>> + clocks = <&pll0>;
>>>>>> + clock-mult = <1>;
>>>>>> + clock-div = <2>;
>>>>>> + clock-output-names = "cpu_clk";
>>>>>> +};
>>>>>> diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
>>>>>> index 1ef271e..85e45f1 100644
>>>>>> --- a/drivers/clk/clk-fixed-factor.c
>>>>>> +++ b/drivers/clk/clk-fixed-factor.c
>>>>>> @@ -11,6 +11,7 @@
>>>>>> #include <linux/clk-provider.h>
>>>>>> #include <linux/slab.h>
>>>>>> #include <linux/err.h>
>>>>>> +#include <linux/of.h>
>>>>>>
>>>>>> /*
>>>>>> * DOC: basic fixed multiplier and divider clock that cannot gate
>>>>>> @@ -96,3 +97,34 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
>>>>>>
>>>>>> return clk;
>>>>>> }
>>>>>> +
>>>>>> +#ifdef CONFIG_OF
>>>>>> +/**
>>>>>> + * of_fixed_factor_clkdiv_setup() - Set up simple fixed factor clock divider
>>>>>> + */
>>>>>> +void of_fixed_factor_clkdiv_setup(struct device_node *node)
>>>>>> +{
>>>>>> + struct clk *clk;
>>>>>> + const char *clk_name = node->name;
>>>>>> + const char *parent_name;
>>>>>> + u32 mult, div;
>>>>>> +
>>>>>> + if (of_property_read_u32(node, "clock-mult", &mult))
>>>>>> + return;
>>>>>> +
>>>>>> + if (of_property_read_u32(node, "clock-div", &div))
>>>>>> + return;
>>>>>> +
>>>>>> + parent_name = of_clk_get_parent_name(node, 0);
>>>>>> +
>>>>>> + of_property_read_string(node, "clock-output-names", &clk_name);
>>>>>> +
>>>>>> + clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0,
>>>>>> + mult, div);
>>>>>> + if (!IS_ERR(clk))
>>>>>> + of_clk_add_provider(node, of_clk_src_simple_get, clk);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(of_fixed_factor_clkdiv_setup);
>>>>>> +CLK_OF_DECLARE(fixed_clkdiv, "fixed-factor-clkdiv",
>>>>>> + of_fixed_factor_clkdiv_setup);
>>>>>> +#endif
>>>>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>>>>>> index 7f197d7..d4937cf 100644
>>>>>> --- a/include/linux/clk-provider.h
>>>>>> +++ b/include/linux/clk-provider.h
>>>>>> @@ -184,6 +184,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
>>>>>> unsigned long fixed_rate);
>>>>>>
>>>>>> void of_fixed_clk_setup(struct device_node *np);
>>>>>> +void of_fixed_factor_clkdiv_setup(struct device_node *node);
>>>>>>
>>>>>> /**
>>>>>> * struct clk_gate - gating clock
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Gregory Clement, Free Electrons
>>>>> Kernel, drivers, real-time and embedded Linux
>>>>> development, consulting, training and support.
>>>>> http://free-electrons.com
>>>>
>>>> --
>>>> Christian Ruppert , <christian.ruppert@xxxxxxxxxx>
>>>> /|
>>>> Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri
>>>> _// | bilis Systems CH-1228 Plan-les-Ouates
>>>
>>
>>
>> --
>> Gregory Clement, Free Electrons
>> Kernel, drivers, real-time and embedded Linux
>> development, consulting, training and support.
>> http://free-electrons.com
>


--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/