Message-ID: <521EF109.3010500@ti.com>
Date:	Thu, 29 Aug 2013 09:58:17 +0300
From:	Tero Kristo <t-kristo@...com>
To:	Mike Turquette <mturquette@...aro.org>
CC:	Kumar Gala <galak@...eaurora.org>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	Matt Sealey <neko@...uhatsu.net>,
	Stephen Boyd <sboyd@...eaurora.org>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Heiko Stübner <heiko@...ech.de>,
	<devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

On 08/29/2013 04:14 AM, Mike Turquette wrote:
> Quoting Kumar Gala (2013-08-28 08:50:10)
>> On Aug 22, 2013, at 12:53 AM, Mike Turquette wrote:
>>
>>> Device Tree binding for the basic clock multiplexer, plus the setup
>>> function to register the clock.  Based on the existing fixed-clock
>>> binding.
>>>
>>> Includes minor beautification of clk-provider.h where some whitespace is
>>> added and of_fixed_factor_clock_setup is relocated to maintain a
>>> consistent style.
>>>
>>> Tero Kristo contributed helpful bug fixes to this patch.
>>>
>>> Signed-off-by: Mike Turquette <mturquette@...aro.org>
>>> Tested-by: Heiko Stuebner <heiko@...ech.de>
>>> Reviewed-by: Heiko Stuebner <heiko@...ech.de>
>>> ---
>>> Changes since v3:
>>> * replaced underscores with dashes in DT property names
>>> * bail from of clock setup function early if of_iomap fails
>>> * removed unecessary explict cast
>>>
>>> .../devicetree/bindings/clock/mux-clock.txt        | 79 ++++++++++++++++++++++
>>> drivers/clk/clk-mux.c                              | 68 ++++++++++++++++++-
>>> include/linux/clk-provider.h                       |  5 +-
>>> 3 files changed, 150 insertions(+), 2 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt b/Documentation/devicetree/bindings/clock/mux-clock.txt
>>> new file mode 100644
>>> index 0000000..21eb3b3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
>>> @@ -0,0 +1,79 @@
>>> +Binding for simple mux clock.
>>> +
>>> +This binding uses the common clock binding[1].  It assumes a
>>> +register-mapped multiplexer with multiple input clock signals or
>>> +parents, one of which can be selected as output.  This clock does not
>>> +gate or adjust the parent rate via a divider or multiplier.
>>> +
>>> +By default the "clocks" property lists the parents in the same order
>>> +as they are programmed into the regster.  E.g:
>>> +
>>> +     clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
>>> +
>>> +results in programming the register as follows:
>>> +
>>> +register value               selected parent clock
>>> +0                    foo_clock
>>> +1                    bar_clock
>>> +2                    baz_clock
>>> +
>>> +Some clock controller IPs do not allow a value of zero to be programmed
>>> +into the register, instead indexing begins at 1.  The optional property
>>> +"index-starts-at-one" modified the scheme as follows:
>>> +
>>> +register value               selected clock parent
>>> +1                    foo_clock
>>> +2                    bar_clock
>>> +3                    baz_clock
>>> +
>>> +Additionally an optional table of bit and parent pairs may be supplied
>>> +like so:
>>> +
>>> +     table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
>>> +
>>
>> I think this is going way too far for what should be in a device tree.  Why should this not just be encoded in tables in the code?
>
> To reduce kernel data bloat.
>
> The mux-clock binding covers a quite a few platforms that have similar
> mux-clock programming requirements. If the DT binding is verbose enough
> then the basic mux clock driver is sufficient to initialize all of the
> mux clocks from DT: no new platform-specific clock driver with a bunch
> of data is necessary.
>
> On the other hand if we rely on tables in C to define how mux-clock
> parents are selected then every platform will have to write their own
> clock driver just to define their clock data.
>
> Having drivers written for the sole purpose of listing out a bunch of
> data sounds like something that DT was meant to solve, even if this
> isn't at the board level and is at the SoC level.

+1. For my work this helps quite a bit at least.

-Tero

>
>>
>>> +where the first value in the pair is the parent clock and the second
>>> +value is the bitfield to be programmed into the register.
>>> +
>>> +The binding must provide the register to control the mux and the mask
>>> +for the corresponding control bits.  Optionally the number of bits to
>>> +shift that mask if necessary.  If the shift value is missing it is the
>>> +same as supplying a zero shift.
>>> +
>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> +
>>> +Required properties:
>>> +- compatible : shall be "mux-clock".
>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>> +- clocks : link phandles of parent clocks
>>> +- reg : base address for register controlling adjustable mux
>>> +- bit-mask : arbitrary bitmask for programming the adjustable mux
>>> +
>>> +Optional properties:
>>> +- clock-output-names : From common clock binding.
>>> +- table : array of integer pairs defining parents & bitfield values
>>> +- bit-shift : number of bits to shift the bit-mask, defaults to
>>> +  (ffs(mask) - 1) if not present
>>> +- index-starts-at-one : valid input select programming starts at 1, not
>>> +  zero
>>> +- hiword-mask : lower half of the register programs the mux, upper half
>>> +  of the register indicates bits that were updated in the lower half
>>> +
>>> +Examples:
>>> +     clock: clock@...08100 {
>>> +             compatible = "mux-clock";
>>> +             #clock-cells = <0>;
>>> +             clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
>>> +             reg = <0x4a008100 0x4>
>>> +             mask = <0x3>;
>>> +             index-starts-at-one;
>>> +     };
>>> +
>>> +     clock: clock@...08100 {
>>> +             #clock-cells = <0>;
>>> +             compatible = "mux-clock";
>>> +             clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
>>> +             reg = <0x4a008100 0x4>;
>>> +             mask = <0x3>;
>>> +             shift = <0>;
>>> +             table = <&clock_foo 1>, <&clock_bar 2>, <&clock_baz 3>;
>>> +     };
>>> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
>>> index 0811633..9292253 100644
>>> --- a/drivers/clk/clk-mux.c
>>> +++ b/drivers/clk/clk-mux.c
>>> @@ -1,7 +1,7 @@
>>> /*
>>>   * Copyright (C) 2011 Sascha Hauer, Pengutronix <s.hauer@...gutronix.de>
>>>   * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@...aro.org>
>>> - * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@...aro.org>
>>> + * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@...aro.org>
>>>   *
>>>   * 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
>>> @@ -16,6 +16,8 @@
>>> #include <linux/slab.h>
>>> #include <linux/io.h>
>>> #include <linux/err.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>>
>>> /*
>>>   * DOC: basic adjustable multiplexer clock that cannot gate
>>> @@ -177,3 +179,67 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
>>>                                      NULL, lock);
>>> }
>>> EXPORT_SYMBOL_GPL(clk_register_mux);
>>> +
>>> +#ifdef CONFIG_OF
>>> +/**
>>> + * of_mux_clk_setup() - Setup function for simple mux rate clock
>>> + */
>>> +void of_mux_clk_setup(struct device_node *node)
>>> +{
>>> +     struct clk *clk;
>>> +     const char *clk_name = node->name;
>>> +     void __iomem *reg;
>>> +     int num_parents;
>>> +     const char **parent_names;
>>> +     int i;
>>> +     u8 clk_mux_flags = 0;
>>> +     u32 mask = 0;
>>> +     u32 shift = 0;
>>> +
>>> +     of_property_read_string(node, "clock-output-names", &clk_name);
>>> +
>>> +     num_parents = of_clk_get_parent_count(node);
>>> +     if (num_parents < 1) {
>>> +             pr_err("%s: mux-clock %s must have parent(s)\n",
>>> +                             __func__, node->name);
>>> +             return;
>>> +     }
>>> +
>>> +     parent_names = kzalloc((sizeof(char*) * num_parents),
>>> +                     GFP_KERNEL);
>>> +
>>> +     for (i = 0; i < num_parents; i++)
>>> +             parent_names[i] = of_clk_get_parent_name(node, i);
>>> +
>>> +     reg = of_iomap(node, 0);
>>> +     if (!reg) {
>>> +             pr_err("%s: no memory mapped for property reg\n", __func__);
>>> +             return;
>>> +     }
>>> +
>>> +     if (of_property_read_u32(node, "bit-mask", &mask)) {
>>> +             pr_err("%s: missing bit-mask property for %s\n", __func__, node->name);
>>> +             return;
>>> +     }
>>> +
>>> +     if (of_property_read_u32(node, "bit-shift", &shift)) {
>>> +             shift = __ffs(mask);
>>> +             pr_debug("%s: bit-shift property defaults to 0x%x for %s\n",
>>> +                             __func__, shift, node->name);
>>> +     }
>>> +
>>> +     if (of_property_read_bool(node, "index-starts-at-one"))
>>> +             clk_mux_flags |= CLK_MUX_INDEX_ONE;
>>> +
>>> +     if (of_property_read_bool(node, "hiword-mask"))
>>> +             clk_mux_flags |= CLK_MUX_HIWORD_MASK;
>>> +
>>> +     clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents,
>>> +                     0, reg, shift, mask, clk_mux_flags, NULL, NULL);
>>> +
>>> +     if (!IS_ERR(clk))
>>> +             of_clk_add_provider(node, of_clk_src_simple_get, clk);
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_mux_clk_setup);
>>> +CLK_OF_DECLARE(mux_clk, "mux-clock", of_mux_clk_setup);
>>> +#endif
>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>>> index 5497739..e019212 100644
>>> --- a/include/linux/clk-provider.h
>>> +++ b/include/linux/clk-provider.h
>>> @@ -350,7 +350,7 @@ struct clk *clk_register_mux_table(struct device *dev, const char *name,
>>>                void __iomem *reg, u8 shift, u32 mask,
>>>                u8 clk_mux_flags, u32 *table, spinlock_t *lock);
>>>
>>> -void of_fixed_factor_clk_setup(struct device_node *node);
>>> +void of_mux_clk_setup(struct device_node *node);
>>>
>>> /**
>>>   * struct clk_fixed_factor - fixed multiplier and divider clock
>>> @@ -371,10 +371,13 @@ struct clk_fixed_factor {
>>> };
>>>
>>> extern struct clk_ops clk_fixed_factor_ops;
>>> +
>>> struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
>>>                const char *parent_name, unsigned long flags,
>>>                unsigned int mult, unsigned int div);
>>>
>>> +void of_fixed_factor_clk_setup(struct device_node *node);
>>> +
>>> /***
>>>   * struct clk_composite - aggregate clock of mux, divider and gate clocks
>>>   *
>>> --
>>> 1.8.1.2
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> --
>> Employee of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

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