Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Kumar Gala]

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@linaro.org>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Heiko Stuebner <heiko@sntech.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?

> +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@4a008100 {
> +		compatible = "mux-clock";
> +		#clock-cells = <0>;
> +		clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> +		reg = <0x4a008100 0x4>
> +		mask = <0x3>;
> +		index-starts-at-one;
> +	};
> +
> +	clock: clock@4a008100 {
> +		#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@pengutronix.de>
>  * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
> - * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
> + * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.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@vger.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

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Mike Turquette]

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@linaro.org>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > Reviewed-by: Heiko Stuebner <heiko@sntech.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.

Regards,
Mike

> 
> > +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@4a008100 {
> > +             compatible = "mux-clock";
> > +             #clock-cells = <0>;
> > +             clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> > +             reg = <0x4a008100 0x4>
> > +             mask = <0x3>;
> > +             index-starts-at-one;
> > +     };
> > +
> > +     clock: clock@4a008100 {
> > +             #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@pengutronix.de>
> >  * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
> > - * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
> > + * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.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@vger.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

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Tero Kristo]

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@linaro.org>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>> Reviewed-by: Heiko Stuebner <heiko@sntech.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@4a008100 {
>>> +             compatible = "mux-clock";
>>> +             #clock-cells = <0>;
>>> +             clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
>>> +             reg = <0x4a008100 0x4>
>>> +             mask = <0x3>;
>>> +             index-starts-at-one;
>>> +     };
>>> +
>>> +     clock: clock@4a008100 {
>>> +             #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@pengutronix.de>
>>>   * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
>>> - * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
>>> + * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.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@vger.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

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [130829 00:06]:
> On 08/29/2013 04:14 AM, Mike Turquette wrote:
> >
> >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.

Yes this is the way to do it. Please don't do drivers where the index
to some data table is passed in device tree. That's going to be a
nightmare in the long run.

The binding should describe a type of hardware like a dpll or a mux,
and then you just define as many instances of those as needed in the
.dts files.

Regards,

Tony

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Kumar Gala]

On Aug 28, 2013, at 8:14 PM, 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@linaro.org>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>> Reviewed-by: Heiko Stuebner <heiko@sntech.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.

Why is this really an issue, for systems that are concerned about kernel size, they are going to be built targeting the HW platform of choice, for one's like a server, the bloat isn't going to be significant, and if so we can look at mechanisms like __init section that would free up after boot.

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

The same could be said of just embedded this info in a C struct that is well known, I don't need the data in the dt for bits and shifts.

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

Why?  If you can have a common mechanism in the device tree there isn't any reason you can't have a common struct that looks similar in C.

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

I just don't see the need for the data to be in the DT.  You can get the same goal w/a standard struct defined in C.

> 
> Regards,
> Mike
> 
>> 
>>> +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@4a008100 {
>>> +             compatible = "mux-clock";
>>> +             #clock-cells = <0>;
>>> +             clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
>>> +             reg = <0x4a008100 0x4>
>>> +             mask = <0x3>;
>>> +             index-starts-at-one;
>>> +     };
>>> +
>>> +     clock: clock@4a008100 {
>>> +             #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@pengutronix.de>
>>> * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
>>> - * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
>>> + * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.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@vger.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

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Mike Turquette]

Quoting Kumar Gala (2013-08-30 13:02:42)
> On Aug 28, 2013, at 8:14 PM, 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@linaro.org>
> >>> Tested-by: Heiko Stuebner <heiko@sntech.de>
> >>> Reviewed-by: Heiko Stuebner <heiko@sntech.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.
> 
> Why is this really an issue, for systems that are concerned about kernel size, they are going to be built targeting the HW platform of choice, for one's like a server, the bloat isn't going to be significant, and if so we can look at mechanisms like __init section that would free up after boot.

s/bloat/churn/

The clock _data_ seems to always have some churn to it. Moving it out to
DT reduces that churn from Linux. My concern above is not about kernel
data size.

> 
> > 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.
> 
> The same could be said of just embedded this info in a C struct that is well known, I don't need the data in the dt for bits and shifts.
> 
> > 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.
> 
> Why?  If you can have a common mechanism in the device tree there isn't any reason you can't have a common struct that looks similar in C.
> 
> > 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.
> 
> I just don't see the need for the data to be in the DT.  You can get the same goal w/a standard struct defined in C.

Absolutely you could put this stuff in C. Originally all of the clock
registration mechanisms in the common clock implementation assumed
static data in the kernel. That's how it used to be.

However DT came along to ARM and the clock framework and these "clock
mapping" bindings started popping up, which are fragile. Adding a clock
requires changes both to the DT binding AND to the kernel. That sucks.

Once again I'll point out that this binding (mux-clock) makes it so that
devices with simpler clock trees do not need to merge any code into the
kernel. What value does device tree bring to me if for every platform
with 8 clocks I have to write a new Linux clock driver with that static
data in it? That also sucks. In that case DT brings almost zero value,
with the exception of linking clock phandles to clock consumers, which
clkdev had been doing for us already.

Regards,
Mike

> 
> > 
> > Regards,
> > Mike
> > 
> >> 
> >>> +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@4a008100 {
> >>> +             compatible = "mux-clock";
> >>> +             #clock-cells = <0>;
> >>> +             clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> >>> +             reg = <0x4a008100 0x4>
> >>> +             mask = <0x3>;
> >>> +             index-starts-at-one;
> >>> +     };
> >>> +
> >>> +     clock: clock@4a008100 {
> >>> +             #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@pengutronix.de>
> >>> * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
> >>> - * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
> >>> + * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.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@vger.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
> 
> -- 
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Kumar Gala]

On Aug 30, 2013, at 3:33 PM, Mike Turquette wrote:

> Quoting Kumar Gala (2013-08-30 13:02:42)
>> On Aug 28, 2013, at 8:14 PM, 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@linaro.org>
>>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.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.
>> 
>> Why is this really an issue, for systems that are concerned about kernel size, they are going to be built targeting the HW platform of choice, for one's like a server, the bloat isn't going to be significant, and if so we can look at mechanisms like __init section that would free up after boot.
> 
> s/bloat/churn/
> 
> The clock _data_ seems to always have some churn to it. Moving it out to
> DT reduces that churn from Linux. My concern above is not about kernel
> data size.

And we have numerous concerns about DTs be non-changeable on shipping systems where kernels still seem to be viewed as acceptable to change.

Not sure I really get this argument about churn.  The churn still exists you've just shifted it.

>>> 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.
>> 
>> The same could be said of just embedded this info in a C struct that is well known, I don't need the data in the dt for bits and shifts.
>> 
>>> 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.
>> 
>> Why?  If you can have a common mechanism in the device tree there isn't any reason you can't have a common struct that looks similar in C.
>> 
>>> 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.
>> 
>> I just don't see the need for the data to be in the DT.  You can get the same goal w/a standard struct defined in C.
> 
> Absolutely you could put this stuff in C. Originally all of the clock
> registration mechanisms in the common clock implementation assumed
> static data in the kernel. That's how it used to be.
> 
> However DT came along to ARM and the clock framework and these "clock
> mapping" bindings started popping up, which are fragile. Adding a clock
> requires changes both to the DT binding AND to the kernel. That sucks.

Sure, we learn things as we see more patterns from how different implementations work.  It seems like you've figured out a good pattern, just don't see why it would need to be in the DT.

> Once again I'll point out that this binding (mux-clock) makes it so that
> devices with simpler clock trees do not need to merge any code into the
> kernel. What value does device tree bring to me if for every platform
> with 8 clocks I have to write a new Linux clock driver with that static
> data in it? That also sucks. In that case DT brings almost zero value,
> with the exception of linking clock phandles to clock consumers, which
> clkdev had been doing for us already.

Do you believe that we will ever get to a point that a new SoC will be supported w/o some new coded added into the kernel?  I don't see that ever happening for embedded systems.

I think this is about where to draw the line, I just very concerned about the pandora's box this potentially opens up.

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Stephen Warren]

On 08/30/2013 02:33 PM, Mike Turquette wrote:
> Quoting Kumar Gala (2013-08-30 13:02:42)
>> On Aug 28, 2013, at 8:14 PM, 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@linaro.org>
>>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.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.
>>
>> Why is this really an issue, for systems that are concerned about kernel size, they are going to be built targeting the HW platform of choice, for one's like a server, the bloat isn't going to be significant, and if so we can look at mechanisms like __init section that would free up after boot.
> 
> s/bloat/churn/
> 
> The clock _data_ seems to always have some churn to it. Moving it out to
> DT reduces that churn from Linux. My concern above is not about kernel
> data size.

That sounds like the opposite of what we should be doing.

It's fine for kernel code/data to change; that's a natural part of
development. Obviously, we should minimize churn, through thorough
review, domain knowledge, etc.

Saying that we must shove the data out into DT because it changes
pre-supposes that we should be changing the DT!

We should strive to write the DT for a particular piece of HW once, and
have it be a complete and accurate description of the HW. That's hard
enough to do with small amounts of simple data. Deliberately pushing
data that's known to be hard to get right and churn a lot into DT seems
to be destined to make most DTs contain incorrect data.

DT-as-an-ABI works both ways; the binding definition needs to stay
constant /and/ the data in any particular DT needs to be accurate too.

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Mike Turquette]

Quoting Stephen Warren (2013-08-30 14:37:46)
> On 08/30/2013 02:33 PM, Mike Turquette wrote:
> > Quoting Kumar Gala (2013-08-30 13:02:42)
> >> On Aug 28, 2013, at 8:14 PM, 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@linaro.org>
> >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
> >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.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.
> >>
> >> Why is this really an issue, for systems that are concerned about kernel size, they are going to be built targeting the HW platform of choice, for one's like a server, the bloat isn't going to be significant, and if so we can look at mechanisms like __init section that would free up after boot.
> > 
> > s/bloat/churn/
> > 
> > The clock _data_ seems to always have some churn to it. Moving it out to
> > DT reduces that churn from Linux. My concern above is not about kernel
> > data size.
> 
> That sounds like the opposite of what we should be doing.
> 
> It's fine for kernel code/data to change; that's a natural part of
> development. Obviously, we should minimize churn, through thorough
> review, domain knowledge, etc.

And with the "clock mapping" style bindings we'll end up changing both
the DT binding definition and the kernel. Not great.

And I'll respond to your points below but the whole "relocate the
problem to DT" argument is simply not my main point. What I want to do
is increase the usefulness of DT by allowing register-level details into
the binding which can 

> 
> Saying that we must shove the data out into DT because it changes
> pre-supposes that we should be changing the DT!
> 
> We should strive to write the DT for a particular piece of HW once, and
> have it be a complete and accurate description of the HW.

We're always trying to do this no matter where the data resides. Having
accurate data is not a new proposal.

> That's hard
> enough to do with small amounts of simple data. Deliberately pushing
> data that's known to be hard to get right and churn a lot into DT seems
> to be destined to make most DTs contain incorrect data.

The data churn doesn't change, whether it's static data in C or nodes in
dts. And for the sane non-mapping-a-clock-to-a-number bindings there is
no cause to change the binding at all when introducing new clock data or
making corrections to the nodes in the dts.

> 
> DT-as-an-ABI works both ways; the binding definition needs to stay
> constant /and/ the data in any particular DT needs to be accurate too.

I'm not proposing changing the binding and accuracy in the dts is
implied. No one has a goal to put inaccurate data into DT.

Regards,
Mike

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Stephen Warren]

On 09/03/2013 05:22 PM, Mike Turquette wrote:
> Quoting Stephen Warren (2013-08-30 14:37:46)
>> On 08/30/2013 02:33 PM, Mike Turquette wrote:
...
>>> The clock _data_ seems to always have some churn to it. Moving it out to
>>> DT reduces that churn from Linux. My concern above is not about kernel
>>> data size.
>>
>> That sounds like the opposite of what we should be doing.
>>
>> It's fine for kernel code/data to change; that's a natural part of
>> development. Obviously, we should minimize churn, through thorough
>> review, domain knowledge, etc.
> 
> And with the "clock mapping" style bindings we'll end up changing both
> the DT binding definition and the kernel. Not great.

What's a "clock mapping" style binding? I guess that means the style
where you have a single DT node that provides multiple clocks, rather
than one DT node per clock?

If the kernel driver changes its internal data, I don't see why that
would have any impact at all on the DT binding definition. We should be
able to use one DT binding definition with arbitrary drivers.

> And I'll respond to your points below but the whole "relocate the
> problem to DT" argument is simply not my main point. What I want to do
> is increase the usefulness of DT by allowing register-level details into
> the binding which can 

Can you expand upon why a DT that encodes register-level details is more
useful? I can't see why there would be any difference in usefulness.

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Mike Turquette]

On Wed, Sep 4, 2013 at 11:36 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/03/2013 05:22 PM, Mike Turquette wrote:
>> Quoting Stephen Warren (2013-08-30 14:37:46)
>>> On 08/30/2013 02:33 PM, Mike Turquette wrote:
> ...
>>>> The clock _data_ seems to always have some churn to it. Moving it out to
>>>> DT reduces that churn from Linux. My concern above is not about kernel
>>>> data size.
>>>
>>> That sounds like the opposite of what we should be doing.
>>>
>>> It's fine for kernel code/data to change; that's a natural part of
>>> development. Obviously, we should minimize churn, through thorough
>>> review, domain knowledge, etc.
>>
>> And with the "clock mapping" style bindings we'll end up changing both
>> the DT binding definition and the kernel. Not great.
>
> What's a "clock mapping" style binding? I guess that means the style
> where you have a single DT node that provides multiple clocks, rather
> than one DT node per clock?
>
> If the kernel driver changes its internal data, I don't see why that
> would have any impact at all on the DT binding definition. We should be
> able to use one DT binding definition with arbitrary drivers.

Yes, I'm referring to a single node providing multiple clocks. As an
example see the Exynos 5420 binding:
Documentation/devicetree/bindings/clock/exynos5420-clock.txt

The clock id's are stored as part of the binding definition resulting
in a mapping scheme that can be fragile. There have already been
patches to fix the id's assigned in the binding, which isn't supposed
to happen because it's a stable interface. If clock phandles are
created by individual nodes in DT then the binding definition need
never be updated due to merge conflicts or renaming which plagues the
mapping scenario.

>
>> And I'll respond to your points below but the whole "relocate the
>> problem to DT" argument is simply not my main point. What I want to do
>> is increase the usefulness of DT by allowing register-level details into
>> the binding which can
>
> Can you expand upon why a DT that encodes register-level details is more
> useful? I can't see why there would be any difference in usefulness.
>

Sure. The usefulness comes out of the fact that we do not need to
maintain data synchronization across dts and clock provider drivers.
The data lives in one place and only one place. We absolutely need a
phandle to a clock in DT link clock consumer devices to their input
clocks, so there is no question that should be in DT. Since we're
already doing that, why not do away with trying to keep dts and C
files in sync and just put all of the data in dts? It's a pure
separation of logic and data. The Linux clock provider driver is
purely logic and no data, which I imagine would appease the mind of
many a computer scientist.

Can you return the favor and tell me why putting register level
details into DT is inherently a bad idea? I'll drop my case if I can
be convinced that putting that level is detail into DT is The Wrong
Way, but I'll need more to go on than tradition and status quo.

Thanks,
Mike

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Stephen Warren]

On 09/05/2013 12:29 PM, Mike Turquette wrote:
> On Wed, Sep 4, 2013 at 11:36 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/03/2013 05:22 PM, Mike Turquette wrote:
>>> Quoting Stephen Warren (2013-08-30 14:37:46)
>>>> On 08/30/2013 02:33 PM, Mike Turquette wrote:
>> ...
>>>>> The clock _data_ seems to always have some churn to it. Moving it out to
>>>>> DT reduces that churn from Linux. My concern above is not about kernel
>>>>> data size.
>>>>
>>>> That sounds like the opposite of what we should be doing.
>>>>
>>>> It's fine for kernel code/data to change; that's a natural part of
>>>> development. Obviously, we should minimize churn, through thorough
>>>> review, domain knowledge, etc.
>>>
>>> And with the "clock mapping" style bindings we'll end up changing both
>>> the DT binding definition and the kernel. Not great.
>>
>> What's a "clock mapping" style binding? I guess that means the style
>> where you have a single DT node that provides multiple clocks, rather
>> than one DT node per clock?
>>
>> If the kernel driver changes its internal data, I don't see why that
>> would have any impact at all on the DT binding definition. We should be
>> able to use one DT binding definition with arbitrary drivers.
> 
> Yes, I'm referring to a single node providing multiple clocks. As an
> example see the Exynos 5420 binding:
> Documentation/devicetree/bindings/clock/exynos5420-clock.txt
> 
> The clock id's are stored as part of the binding definition resulting
> in a mapping scheme that can be fragile.

The mapping shouldn't be fragile if e.g.
include/dt-bindings/clock/exynos5420.h were used to define the values.
That way, both the Exynos clock driver and Exynos DT files could both
include the header, and would always be in sync.

> There have already been
> patches to fix the id's assigned in the binding, which isn't supposed
> to happen because it's a stable interface.

That's definitely a real problem. The values should be stable.
Preferably, the values should be derived from some aspect of the HW, and
hence be stable.

For example, many clock IDs on Tegra are derived from the clock's bit
index within the peripheral clock enable registers. Although I must
admit we have a bit of a mess in the Tegra clocks w.r.t. mis-using clock
IDs for reset IDs and hence there are some peripheral clock IDS that
don't map 1:1 with the register, and there are other clocks which aren't
peripheral clocksthat we've assigned arbitrary IDs to rather than some
HW-derived ID.

Alternatively, perhaps a register address unique to the clock could be used.

If new values are added, the additions should all happen in a single
tree, and hence can be co-ordinated, thus avoiding any merge-conflicts.

Even ignoring HW-derived clock IDs, people writing DT bindings simply
need to get used to bindings being an ABI, and put extra effort into
making sure the list of clocks is accurate and complete.

Finally, while it's true that a DT binding definition is an ABI, and
perhaps DT content isn't (so if there's a DT content bug it can simply
be fixed), if DT is wrong because of insufficient thought about its
content, it's still wrong, and the system doesn't work correctly.
Whether we edit a kernel clock driver or a DT file to solve a problem,
there was still a problem. Placing the data into DT doesn't make it any
less likely there will be a problem if sufficient care isn't taken when
thinking about the clock structure.

> If clock phandles are
> created by individual nodes in DT then the binding definition need
> never be updated due to merge conflicts or renaming which plagues the
> mapping scenario.

That's true.

But if we take that approach, shouldn't we just ban #clock-cells?

The only case #clock-cells would still be legitimate would be an array
of identical clocks represented by a single node, and even then the
argument could be extended so say: just write out a node for each clock
in the array, just like if the clocks weren't in an array or were
different types.

>>> And I'll respond to your points below but the whole "relocate the
>>> problem to DT" argument is simply not my main point. What I want to do
>>> is increase the usefulness of DT by allowing register-level details into
>>> the binding which can
>>
>> Can you expand upon why a DT that encodes register-level details is more
>> useful? I can't see why there would be any difference in usefulness.
> 
> Sure. The usefulness comes out of the fact that we do not need to
> maintain data synchronization across dts and clock provider drivers.

Only the clock IDs. That's a very small amount of information. And
synchronizing the two simply means including a header file that defines
the IDs in both places. This is *exactly* why I created the
include/dt-bindings/ directory, to house such header files.

> The data lives in one place and only one place. We absolutely need a
> phandle to a clock in DT link clock consumer devices to their input
> clocks, so there is no question that should be in DT. Since we're
> already doing that, why not do away with trying to keep dts and C
> files in sync and just put all of the data in dts? It's a pure
> separation of logic and data. The Linux clock provider driver is
> purely logic and no data, which I imagine would appease the mind of
> many a computer scientist.

Separation of code and data is good. However, one can achieve that
simply by putting data into C structs/arrays, without having to parse it
out of DT.

> Can you return the favor and tell me why putting register level
> details into DT is inherently a bad idea? I'll drop my case if I can
> be convinced that putting that level is detail into DT is The Wrong
> Way, but I'll need more to go on than tradition and status quo.

Here are a few reasons, in no particular order:

1)

At least for large SoCs (rather than e.g. a simple clock generator
chip/crystal with 1 or 2 outputs), clock drivers need a *lot* of data.
We can either:

a) Put that data into the clock driver, so it's "just there" for the
clock driver to use.

b) Represent that data in DT, and write code in the clock driver to
parse DT when the driver probe()s.

Option (a) is very simple.

Option (b) entails writing (and executing) a whole bunch of DT parsing
code. It's a lot of effort to define the DT binding for the data,
convert the data into DT format, and write the parsing code. It wastes
execution time at boot. In the end, the result of the parsing is exactly
the same data structures that could have been embedded into DT in the
first place. This seems like a futile effort.

2)

If the clock driver knows it's running on e.g. Tegra20 vs. Tegra30, that
information alone is enough to fully describe all details of the clock
tree present within the SoC. That information is cast in stone in the
SoC HW design.

Philosophically, I believe DT should be used to describe what varies
between different uses of a HW block, not the internals of a HW block
itself. This means describing the environment around an IP block (e.g.
which interrupts sinks an I2C controller is connected to, which GPIOs a
board used for an SD card CD line), rather than the internals of a
block, which are completely fixed.

For clocks, this means that the routing of a clock module's
inputs/outputs are useful to describe in DT, since they may be connected
differently depending on which SoC a clock module is placed into, or
which board an SoC is placed into. However, the HW within the block is
fixed, so doesn't need to be represented by a data structure whose
intent is to represent environmental differences.

To be honest, I would rather not even put e.g. on-SoC I2C, SPI, SDHCI
controllers into DT, since they are 100% defined by the top-level SoC
node. However, in practice we must put those nodes into DT for a few
reasons:

a) They act as a container for the I2C/SPI devices that are connected to
them, so at least something has to exist in DT.

b) There's some board-specific parameterization of those controllers
such as max bus clock rate for I2C/SPI, bus width for SDHCI, which GPIOs
are used for CD/WP for SDHCI, or even whether the controller is
enabled/disabled.

c) Some of the resources those controllers use (IRQs, GPIOs) may also be
used by board-specific entities (e.g. off-SoC IRQ source or GPIO sink).
The on-SoC devices appear in DT in order to allow representation of the
IRQs/GPIOs they use in a consistent manner with off-SoCs devices, for
simplicity.

As such, we end up treating them much like any other off-SoC device in
terms of representing them as a DT node.

3)

Why are clocks a special case?

A "simple-gpio" controller binding and driver was proposed, and we had a
very similar conversation to this one then. I believe simple-gpio was
rejected for the reasons I presented above.

pintctrl-simple was initially rejected because it would have ended up
being essentially a very complex list of (register, value) writes that
the kernel performed at bootup. I'm not sure how pinctrl-simple finally
got accepted into the kernel; I think people were just busy and didn't
notice it and hence didn't object:-(

If we take the "DT should represent the register details" argument to
extreme, why not have some hyperbole? :-)

a) Do it for everything. For example, Tegra20 and Tegra30 I2S are
semantically the same, but with register offsets moved around rather
randomly. Perhaps we should have a mapping table of register field name
to offset, bit position, and size in DT, and some automated abstraction
layer in the kernel to parse this and re-direct all the register IO so
we can use a single driver.

To me this sounds a bit ridiculous, whereas putting all the clock
register details in DT perhaps doesn't (at least depending on exactly
what that ends up meaning). However, they're really exactly the same thing.

b) Why have drivers at all? Shouldn't the kernel just manage the core
ARM CPU, memory, MMU, and other standardized low-level tasks, yet all
drivers should be written in Forth with the byte-code stored in DT and
evaluated by the kernel instead? This even separates the driver code out
of the kernel, and really reduces churn:-)

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Sylwester Nawrocki]

On 09/05/2013 10:30 PM, Stephen Warren wrote:
> On 09/05/2013 12:29 PM, Mike Turquette wrote:
>> >  On Wed, Sep 4, 2013 at 11:36 AM, Stephen Warren<swarren@wwwdotorg.org>  wrote:
>>> >>  On 09/03/2013 05:22 PM, Mike Turquette wrote:
>>>> >>>  Quoting Stephen Warren (2013-08-30 14:37:46)
>>>>> >>>>  On 08/30/2013 02:33 PM, Mike Turquette wrote:
>>> >>  ...
>>>>>> >>>>>  The clock_data_  seems to always have some churn to it. Moving it out to
>>>>>> >>>>>  DT reduces that churn from Linux. My concern above is not about kernel
>>>>>> >>>>>  data size.
>>>>> >>>>
>>>>> >>>>  That sounds like the opposite of what we should be doing.
>>>>> >>>>
>>>>> >>>>  It's fine for kernel code/data to change; that's a natural part of
>>>>> >>>>  development. Obviously, we should minimize churn, through thorough
>>>>> >>>>  review, domain knowledge, etc.
>>>> >>>
>>>> >>>  And with the "clock mapping" style bindings we'll end up changing both
>>>> >>>  the DT binding definition and the kernel. Not great.
>>> >>
>>> >>  What's a "clock mapping" style binding? I guess that means the style
>>> >>  where you have a single DT node that provides multiple clocks, rather
>>> >>  than one DT node per clock?
>>> >>
>>> >>  If the kernel driver changes its internal data, I don't see why that
>>> >>  would have any impact at all on the DT binding definition. We should be
>>> >>  able to use one DT binding definition with arbitrary drivers.
>> >
>> >  Yes, I'm referring to a single node providing multiple clocks. As an
>> >  example see the Exynos 5420 binding:
>> >  Documentation/devicetree/bindings/clock/exynos5420-clock.txt
>> >
>> >  The clock id's are stored as part of the binding definition resulting
>> >  in a mapping scheme that can be fragile.
>
> The mapping shouldn't be fragile if e.g.
> include/dt-bindings/clock/exynos5420.h were used to define the values.
> That way, both the Exynos clock driver and Exynos DT files could both
> include the header, and would always be in sync.

It was our intention to have things done this way since first time the idea
of the preprocessor support in dtc was proposed, and since very initial
versions of the exynos clocks driver. I took some time but there have been
recently already posted patches moving the values definition to common
headers [1].

[1] http://www.spinics.net/lists/arm-kernel/msg271807.html

--
Thanks,
Sylwester

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Tero Kristo]

Hi,

Chirping in my thoughts below.

On 09/05/2013 11:30 PM, Stephen Warren wrote:
> On 09/05/2013 12:29 PM, Mike Turquette wrote:
>> On Wed, Sep 4, 2013 at 11:36 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 09/03/2013 05:22 PM, Mike Turquette wrote:
>>>> Quoting Stephen Warren (2013-08-30 14:37:46)
>>>>> On 08/30/2013 02:33 PM, Mike Turquette wrote:
>>> ...
>>>>>> The clock _data_ seems to always have some churn to it. Moving it out to
>>>>>> DT reduces that churn from Linux. My concern above is not about kernel
>>>>>> data size.
>>>>>
>>>>> That sounds like the opposite of what we should be doing.
>>>>>
>>>>> It's fine for kernel code/data to change; that's a natural part of
>>>>> development. Obviously, we should minimize churn, through thorough
>>>>> review, domain knowledge, etc.
>>>>
>>>> And with the "clock mapping" style bindings we'll end up changing both
>>>> the DT binding definition and the kernel. Not great.
>>>
>>> What's a "clock mapping" style binding? I guess that means the style
>>> where you have a single DT node that provides multiple clocks, rather
>>> than one DT node per clock?
>>>
>>> If the kernel driver changes its internal data, I don't see why that
>>> would have any impact at all on the DT binding definition. We should be
>>> able to use one DT binding definition with arbitrary drivers.
>>
>> Yes, I'm referring to a single node providing multiple clocks. As an
>> example see the Exynos 5420 binding:
>> Documentation/devicetree/bindings/clock/exynos5420-clock.txt
>>
>> The clock id's are stored as part of the binding definition resulting
>> in a mapping scheme that can be fragile.
>
> The mapping shouldn't be fragile if e.g.
> include/dt-bindings/clock/exynos5420.h were used to define the values.
> That way, both the Exynos clock driver and Exynos DT files could both
> include the header, and would always be in sync.
>
>> There have already been
>> patches to fix the id's assigned in the binding, which isn't supposed
>> to happen because it's a stable interface.
>
> That's definitely a real problem. The values should be stable.
> Preferably, the values should be derived from some aspect of the HW, and
> hence be stable.
>
> For example, many clock IDs on Tegra are derived from the clock's bit
> index within the peripheral clock enable registers. Although I must
> admit we have a bit of a mess in the Tegra clocks w.r.t. mis-using clock
> IDs for reset IDs and hence there are some peripheral clock IDS that
> don't map 1:1 with the register, and there are other clocks which aren't
> peripheral clocksthat we've assigned arbitrary IDs to rather than some
> HW-derived ID.
>
> Alternatively, perhaps a register address unique to the clock could be used.
>
> If new values are added, the additions should all happen in a single
> tree, and hence can be co-ordinated, thus avoiding any merge-conflicts.
>
> Even ignoring HW-derived clock IDs, people writing DT bindings simply
> need to get used to bindings being an ABI, and put extra effort into
> making sure the list of clocks is accurate and complete.
>
> Finally, while it's true that a DT binding definition is an ABI, and
> perhaps DT content isn't (so if there's a DT content bug it can simply
> be fixed), if DT is wrong because of insufficient thought about its
> content, it's still wrong, and the system doesn't work correctly.
> Whether we edit a kernel clock driver or a DT file to solve a problem,
> there was still a problem. Placing the data into DT doesn't make it any
> less likely there will be a problem if sufficient care isn't taken when
> thinking about the clock structure.
>
>> If clock phandles are
>> created by individual nodes in DT then the binding definition need
>> never be updated due to merge conflicts or renaming which plagues the
>> mapping scenario.
>
> That's true.
>
> But if we take that approach, shouldn't we just ban #clock-cells?
>
> The only case #clock-cells would still be legitimate would be an array
> of identical clocks represented by a single node, and even then the
> argument could be extended so say: just write out a node for each clock
> in the array, just like if the clocks weren't in an array or were
> different types.
>
>>>> And I'll respond to your points below but the whole "relocate the
>>>> problem to DT" argument is simply not my main point. What I want to do
>>>> is increase the usefulness of DT by allowing register-level details into
>>>> the binding which can
>>>
>>> Can you expand upon why a DT that encodes register-level details is more
>>> useful? I can't see why there would be any difference in usefulness.
>>
>> Sure. The usefulness comes out of the fact that we do not need to
>> maintain data synchronization across dts and clock provider drivers.
>
> Only the clock IDs. That's a very small amount of information. And
> synchronizing the two simply means including a header file that defines
> the IDs in both places. This is *exactly* why I created the
> include/dt-bindings/ directory, to house such header files.
>
>> The data lives in one place and only one place. We absolutely need a
>> phandle to a clock in DT link clock consumer devices to their input
>> clocks, so there is no question that should be in DT. Since we're
>> already doing that, why not do away with trying to keep dts and C
>> files in sync and just put all of the data in dts? It's a pure
>> separation of logic and data. The Linux clock provider driver is
>> purely logic and no data, which I imagine would appease the mind of
>> many a computer scientist.
>
> Separation of code and data is good. However, one can achieve that
> simply by putting data into C structs/arrays, without having to parse it
> out of DT.
>
>> Can you return the favor and tell me why putting register level
>> details into DT is inherently a bad idea? I'll drop my case if I can
>> be convinced that putting that level is detail into DT is The Wrong
>> Way, but I'll need more to go on than tradition and status quo.
>
> Here are a few reasons, in no particular order:
>
> 1)
>
> At least for large SoCs (rather than e.g. a simple clock generator
> chip/crystal with 1 or 2 outputs), clock drivers need a *lot* of data.
> We can either:
>
> a) Put that data into the clock driver, so it's "just there" for the
> clock driver to use.
>
> b) Represent that data in DT, and write code in the clock driver to
> parse DT when the driver probe()s.
>
> Option (a) is very simple.

How can you claim option (a) to be very simple compared to (b)? I think 
both are as easy / or hard to implement.

> Option (b) entails writing (and executing) a whole bunch of DT parsing
> code.It's a lot of effort to define the DT binding for the data,
> convert the data into DT format, and write the parsing code. It wastes
> execution time at boot. In the end, the result of the parsing is exactly
> the same data structures that could have been embedded into DT in the
> first place. This seems like a futile effort.

Not really, consider a new SoC where you don't have any kind of data at 
all. You need to write the data tables anyway, whether they are under DT 
or some kernel data struct. The execution time remain in place for 
parsing DT data though, but I wouldn't think this to be a problem. Also, 
you should consider multiarch ARM kernel, where same kernel binary 
should support multiple SoCs, this would entail having clock data for 
all built in to the kernel, which can be a problem also. It just boils 
down to balancing things between execution time and memory cost, which 
IMO, kernel should not decide, but should be decided by people who use 
the kernel for whatever purpose it may be.

>
> 2)
>
> If the clock driver knows it's running on e.g. Tegra20 vs. Tegra30, that
> information alone is enough to fully describe all details of the clock
> tree present within the SoC. That information is cast in stone in the
> SoC HW design.
>
> Philosophically, I believe DT should be used to describe what varies
> between different uses of a HW block, not the internals of a HW block
> itself. This means describing the environment around an IP block (e.g.
> which interrupts sinks an I2C controller is connected to, which GPIOs a
> board used for an SD card CD line), rather than the internals of a
> block, which are completely fixed.
>
> For clocks, this means that the routing of a clock module's
> inputs/outputs are useful to describe in DT, since they may be connected
> differently depending on which SoC a clock module is placed into, or
> which board an SoC is placed into. However, the HW within the block is
> fixed, so doesn't need to be represented by a data structure whose
> intent is to represent environmental differences.

You can just as easily claim that anything internal to SoC should be 
left out from DT, as this is cast in stone (or rather, silicon) also. We 
should only use it to describe board layout. Everything else, the kernel 
can 'know' by compile time.

>
> To be honest, I would rather not even put e.g. on-SoC I2C, SPI, SDHCI
> controllers into DT, since they are 100% defined by the top-level SoC
> node. However, in practice we must put those nodes into DT for a few
> reasons:
>
> a) They act as a container for the I2C/SPI devices that are connected to
> them, so at least something has to exist in DT.
>
> b) There's some board-specific parameterization of those controllers
> such as max bus clock rate for I2C/SPI, bus width for SDHCI, which GPIOs
> are used for CD/WP for SDHCI, or even whether the controller is
> enabled/disabled.
>
> c) Some of the resources those controllers use (IRQs, GPIOs) may also be
> used by board-specific entities (e.g. off-SoC IRQ source or GPIO sink).
> The on-SoC devices appear in DT in order to allow representation of the
> IRQs/GPIOs they use in a consistent manner with off-SoCs devices, for
> simplicity.
>
> As such, we end up treating them much like any other off-SoC device in
> terms of representing them as a DT node.
>
> 3)
>
> Why are clocks a special case?
>
> A "simple-gpio" controller binding and driver was proposed, and we had a
> very similar conversation to this one then. I believe simple-gpio was
> rejected for the reasons I presented above.
>
> pintctrl-simple was initially rejected because it would have ended up
> being essentially a very complex list of (register, value) writes that
> the kernel performed at bootup. I'm not sure how pinctrl-simple finally
> got accepted into the kernel; I think people were just busy and didn't
> notice it and hence didn't object:-(
>
> If we take the "DT should represent the register details" argument to
> extreme, why not have some hyperbole? :-)
>
> a) Do it for everything. For example, Tegra20 and Tegra30 I2S are
> semantically the same, but with register offsets moved around rather
> randomly. Perhaps we should have a mapping table of register field name
> to offset, bit position, and size in DT, and some automated abstraction
> layer in the kernel to parse this and re-direct all the register IO so
> we can use a single driver.
>
> To me this sounds a bit ridiculous, whereas putting all the clock
> register details in DT perhaps doesn't (at least depending on exactly
> what that ends up meaning). However, they're really exactly the same thing.
>
> b) Why have drivers at all? Shouldn't the kernel just manage the core
> ARM CPU, memory, MMU, and other standardized low-level tasks, yet all
> drivers should be written in Forth with the byte-code stored in DT and
> evaluated by the kernel instead? This even separates the driver code out
> of the kernel, and really reduces churn:-)

I can turn this around, as you went to this road. Why have DT at all? 
Personally I hate the whole idea of a devicetree, however am forced to 
use it as somebody decided it is a good idea to have one. It doesn't 
really solve any problems, it just creates new ones in a way of 
political BS where everybody claims they know how DT should be used, and 
this just prevents people from actually using it at all. Also, it 
creates just one new unnecessary dependency for boot, previously we had 
bootloader and kernel images which had to be in sync, now we have 
bootloader + DT + kernel. What next? Maybe we should move the clock data 
into a firmware file of its own?

Why do you care so much what actually goes into the devicetree? 
Shouldn't people be let use it how they see fit? For the clock bindings 
business this is the same, even if the bindings are there, you are free 
to use them if you like, and if you don't like them, you can do things 
differently.

Personally, I have large amount of code which depends on these basic 
clock bindings right now, and would like to see them go forward. I can 
of course go back and convert the code to such format that everything is 
as static tables under kernel, but in that case I don't think I need DT 
for anything. Also, then people start to complain again that you should 
move stuff to DT. Grrrr.... :)

-Tero

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Stephen Warren]

On 09/06/2013 12:53 AM, Tero Kristo wrote:
> On 09/05/2013 11:30 PM, Stephen Warren wrote:

...
>> 1)
>>
>> At least for large SoCs (rather than e.g. a simple clock generator
>> chip/crystal with 1 or 2 outputs), clock drivers need a *lot* of data.
>> We can either:
>>
>> a) Put that data into the clock driver, so it's "just there" for the
>> clock driver to use.
>>
>> b) Represent that data in DT, and write code in the clock driver to
>> parse DT when the driver probe()s.
>>
>> Option (a) is very simple.
> 
> How can you claim option (a) to be very simple compared to (b)? I think
> both are as easy / or hard to implement.

Well, the work required for (b) is a pure super-set of the work require
for (a), so clearly (a) is less work (perhaps the issue you're debating
is harder/easier rather than more/less work?)

>> Option (b) entails writing (and executing) a whole bunch of DT parsing
>> code.It's a lot of effort to define the DT binding for the data,
>> convert the data into DT format, and write the parsing code. It wastes
>> execution time at boot. In the end, the result of the parsing is exactly
>> the same data structures that could have been embedded into DT in the
>> first place. This seems like a futile effort.
> 
> Not really, consider a new SoC where you don't have any kind of data at
> all. You need to write the data tables anyway, whether they are under DT
> or some kernel data struct.

Yes.

But beyond writing the data tables, you also don't/do have to write all
the DT parsing code based on choosing (a) or (b), etc.

> The execution time remain in place for
> parsing DT data though, but I wouldn't think this to be a problem. Also,
> you should consider multiarch ARM kernel, where same kernel binary
> should support multiple SoCs, this would entail having clock data for
> all built in to the kernel, which can be a problem also.

There's no reason that the clock data has to be built into the kernel at
all; we should support even SoC clock drivers as modules in an initrd.
Alternatively, drop the unused data from the kernel after boot via
__init or similar. Alternatively, "pre-link" the clock driver module
into the kernel in a way that allows it to be unloaded after boot even
though it was built-in.

...
> You can just as easily claim that anything internal to SoC should be
> left out from DT, as this is cast in stone (or rather, silicon) also. We
> should only use it to describe board layout. Everything else, the kernel
> can 'know' by compile time.

I did, immediately below:-) And then I went on to explain why that's
necessary in many cases.

...
> I can turn this around, as you went to this road. Why have DT at all?

I believe (at least for ARM) the idea was to avoid churn to the kernel
for supporting the numerous different *boards*.

The kernel needs and contains drivers for HW blocks, and so since
they're there, they may as well encode everything about the HW block.

However, in most cases, the kernel shouldn't contain drivers for boards;
boards are built from various common components which have drivers. DT
is used to describe how those components are inter-connected. Hence, we
can hopefully remove all board-related churn from the kernel (once the
DT files are moved out of the kernel).

> Personally I hate the whole idea of a devicetree, however am forced to
> use it as somebody decided it is a good idea to have one. It doesn't
> really solve any problems, it just creates new ones in a way of
> political BS where everybody claims they know how DT should be used, and
> this just prevents people from actually using it at all. Also, it
> creates just one new unnecessary dependency for boot, previously we had
> bootloader and kernel images which had to be in sync, now we have
> bootloader + DT + kernel. What next? Maybe we should move the clock data
> into a firmware file of its own?

Well, I can sympathize, but I think the time is past for debating that.

> Why do you care so much what actually goes into the devicetree?

To get DT right.

Even if we went back to board files and mach-xxx specific code rather
than cleanly separated drivers, it would still be beneficial to have
much more oversight of board/mach-xxx code than there was previously.
Board files made it very easy to do SoC-specific hacks. To avoid that,
in either DT or board files, we're trying to impose standards so that we
pick correct, generic, appropriate solutions, rather than letting
everyone run of with isolated ad-hoc solutions.

> Shouldn't people be let use it how they see fit? For the clock bindings
> business this is the same, even if the bindings are there, you are free
> to use them if you like, and if you don't like them, you can do things
> differently.

We'd be better of creating as much standardization as possible, so that
all SoCs/boards/.. work as similarly as possible, and we achieve maximal
code reuse, design-reuse, and allow people to understand everything
rather than just one particular SoC's/board's solution.

If we don't get some re-use and standardization out of DT, we really may
as well just use board files.

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Saravana Kannan]

On 09/06/2013 12:01 PM, Stephen Warren wrote:
> On 09/06/2013 12:53 AM, Tero Kristo wrote:
>> On 09/05/2013 11:30 PM, Stephen Warren wrote:
>
> ...
>>> 1)
>>>
>>> At least for large SoCs (rather than e.g. a simple clock generator
>>> chip/crystal with 1 or 2 outputs), clock drivers need a *lot* of data.
>>> We can either:
>>>
>>> a) Put that data into the clock driver, so it's "just there" for the
>>> clock driver to use.
>>>
>>> b) Represent that data in DT, and write code in the clock driver to
>>> parse DT when the driver probe()s.
>>>
>>> Option (a) is very simple.
>>
>> How can you claim option (a) to be very simple compared to (b)? I think
>> both are as easy / or hard to implement.
>
> Well, the work required for (b) is a pure super-set of the work require
> for (a), so clearly (a) is less work (perhaps the issue you're debating
> is harder/easier rather than more/less work?)
>
>>> Option (b) entails writing (and executing) a whole bunch of DT parsing
>>> code.It's a lot of effort to define the DT binding for the data,
>>> convert the data into DT format, and write the parsing code. It wastes
>>> execution time at boot. In the end, the result of the parsing is exactly
>>> the same data structures that could have been embedded into DT in the
>>> first place. This seems like a futile effort.
>>
>> Not really, consider a new SoC where you don't have any kind of data at
>> all. You need to write the data tables anyway, whether they are under DT
>> or some kernel data struct.
>
> Yes.
>
> But beyond writing the data tables, you also don't/do have to write all
> the DT parsing code based on choosing (a) or (b), etc.
>
>> The execution time remain in place for
>> parsing DT data though, but I wouldn't think this to be a problem. Also,
>> you should consider multiarch ARM kernel, where same kernel binary
>> should support multiple SoCs, this would entail having clock data for
>> all built in to the kernel, which can be a problem also.
>
> There's no reason that the clock data has to be built into the kernel at
> all; we should support even SoC clock drivers as modules in an initrd.
> Alternatively, drop the unused data from the kernel after boot via
> __init or similar. Alternatively, "pre-link" the clock driver module
> into the kernel in a way that allows it to be unloaded after boot even
> though it was built-in.
>
> ...
>> You can just as easily claim that anything internal to SoC should be
>> left out from DT, as this is cast in stone (or rather, silicon) also. We
>> should only use it to describe board layout. Everything else, the kernel
>> can 'know' by compile time.
>
> I did, immediately below:-) And then I went on to explain why that's
> necessary in many cases.
>
> ...
>> I can turn this around, as you went to this road. Why have DT at all?
>
> I believe (at least for ARM) the idea was to avoid churn to the kernel
> for supporting the numerous different *boards*.
>
> The kernel needs and contains drivers for HW blocks, and so since
> they're there, they may as well encode everything about the HW block.
>
> However, in most cases, the kernel shouldn't contain drivers for boards;
> boards are built from various common components which have drivers. DT
> is used to describe how those components are inter-connected. Hence, we
> can hopefully remove all board-related churn from the kernel (once the
> DT files are moved out of the kernel).
>
>> Personally I hate the whole idea of a devicetree, however am forced to
>> use it as somebody decided it is a good idea to have one. It doesn't
>> really solve any problems, it just creates new ones in a way of
>> political BS where everybody claims they know how DT should be used, and
>> this just prevents people from actually using it at all. Also, it
>> creates just one new unnecessary dependency for boot, previously we had
>> bootloader and kernel images which had to be in sync, now we have
>> bootloader + DT + kernel. What next? Maybe we should move the clock data
>> into a firmware file of its own?
>
> Well, I can sympathize, but I think the time is past for debating that.
>
>> Why do you care so much what actually goes into the devicetree?
>
> To get DT right.
>
> Even if we went back to board files and mach-xxx specific code rather
> than cleanly separated drivers, it would still be beneficial to have
> much more oversight of board/mach-xxx code than there was previously.
> Board files made it very easy to do SoC-specific hacks. To avoid that,
> in either DT or board files, we're trying to impose standards so that we
> pick correct, generic, appropriate solutions, rather than letting
> everyone run of with isolated ad-hoc solutions.
>
>> Shouldn't people be let use it how they see fit? For the clock bindings
>> business this is the same, even if the bindings are there, you are free
>> to use them if you like, and if you don't like them, you can do things
>> differently.
>
> We'd be better of creating as much standardization as possible, so that
> all SoCs/boards/.. work as similarly as possible, and we achieve maximal
> code reuse, design-reuse, and allow people to understand everything
> rather than just one particular SoC's/board's solution.
>
> If we don't get some re-use and standardization out of DT, we really may
> as well just use board files.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

I didn't read the full thread, but I have talked about this several 
times with Mike and Stephen Boyd before.

Here's my view on this.

I think the stance of saying the entire clock tree data should not be in 
DT is rather absurd and very contradictory to the existing clock DT 
bindings.

There are two ways to look at how the clock bindings should be:

1) We only care about the clocks that are sent out of a clock controller 
(CC) HW to other blocks. We don't care about the internals of the CC. In 
that case, the DT bindings should not have any details about the clock, 
Just a list of names/ids of the clocks being sent out of the HW block so 
that the clock provider driver for that CC knows which clock is being 
clk_get()'ed. The DT shouldn't specify if it's a frickin PLL, mux, 
divider, gate clock, etc. None of the clients care what is the type of 
the clock, so why list all that data in DT?

2) We want to document the entire clock tree data in DT so that we don't 
have to keep adding C files just to capture the data needed to represent 
the clock tree when the actual code can be 100% reused across several 
chips. Even in this case, the clients in DT will only care about the 
clock and not what their type/rest of the data is. So, we will still 
need to have a list of nodes/phandles (not actual linux devices), names 
or ids.

But the current clock DT binding is a Frankenstein monster of (1) and 
(2) that nobody loves. We list the type and other details of the clock 
leaf nodes, but not the rest of the tree. So, that additional data is 
useless both to the clients and to the CC driver. There's no benefit to 
the CC driver in knowing only the partial details of a partial list of 
clocks (leaf nodes). Would rather pick (1) or (2) instead of the current 
abomination.

Sure, someone will come and argue, "Oh, but my clock tree is simple, so 
I can still use just the data and have a portable driver". But that's a 
very narrow outlook that doesn't scale and work for everyone. And even 
in that case, I'm fairly certain they are hacking some stuff up and not 
now truly representing the entire HW clock tree using the clock 
framework. I mean, which HW has a clock "tree" with just leaf nodes -- 
that can only be possible if you have one XO (crystal oscillator) for 
every clock and none of the clock rates are changeable.

And finally to give my preference, I prefer option (2) that represents 
the entire clock tree in DT and here are the reasons:

A) Wasn't one of the main reasons for ARM using DT to stop code churn 
for minor changes in HW? I even vaguely remember Linux co,plaining that 
all us stupid clock folks keeps making code changes to update minor 
data. So, why are we trying to pick DT bindings that will continue to 
cause a shit ton of code churn?

B) Also, don't we always complain that HW vendors aren't documenting 
their HW and opening it up? What better way that describing the entire 
HW block in DT? It gives you all the details you about it. So, why are 
we pushing against it? This just seems an arbitrary push back.

C) Clocks change often and in minor ways between chips from the same 
vendor. DT is a lot more capable and less repetitive than C to list the 
"diffs". In DT, we can include the clock tree data from another chip and 
just "fix up" the parts that changed. This would be so much nicer than 
creating separate C files for each chip or trying to update the 
structures at runtime based on some vendor specific DT property or 
compatible string.

Btw, this is the kind of stuff I want to discuss in ARM Summit, but I'm 
still waiting for an invite :( Hopefully it will come soon enough that I 
don't miss the ARM Summit because I don't have time to get a UK Visa.

Thanks,
Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

[Tomasz Figa]

On Friday 06 of September 2013 13:01:15 Stephen Warren wrote:
> On 09/06/2013 12:53 AM, Tero Kristo wrote:
> > On 09/05/2013 11:30 PM, Stephen Warren wrote:
> ...
> 
> >> 1)
> >> 
> >> At least for large SoCs (rather than e.g. a simple clock generator
> >> chip/crystal with 1 or 2 outputs), clock drivers need a *lot* of
> >> data.
> >> We can either:
> >> 
> >> a) Put that data into the clock driver, so it's "just there" for the
> >> clock driver to use.
> >> 
> >> b) Represent that data in DT, and write code in the clock driver to
> >> parse DT when the driver probe()s.
> >> 
> >> Option (a) is very simple.
> > 
> > How can you claim option (a) to be very simple compared to (b)? I
> > think
> > both are as easy / or hard to implement.
> 
> Well, the work required for (b) is a pure super-set of the work require
> for (a), so clearly (a) is less work (perhaps the issue you're debating
> is harder/easier rather than more/less work?)

+1

> >> Option (b) entails writing (and executing) a whole bunch of DT
> >> parsing
> >> code.It's a lot of effort to define the DT binding for the data,
> >> convert the data into DT format, and write the parsing code. It
> >> wastes
> >> execution time at boot. In the end, the result of the parsing is
> >> exactly the same data structures that could have been embedded into
> >> DT in the first place. This seems like a futile effort.
> > 
> > Not really, consider a new SoC where you don't have any kind of data
> > at
> > all. You need to write the data tables anyway, whether they are under
> > DT or some kernel data struct.
> 
> Yes.
> 
> But beyond writing the data tables, you also don't/do have to write all
> the DT parsing code based on choosing (a) or (b), etc.

+1

> > The execution time remain in place for
> > parsing DT data though, but I wouldn't think this to be a problem.
> > Also, you should consider multiarch ARM kernel, where same kernel
> > binary should support multiple SoCs, this would entail having clock
> > data for all built in to the kernel, which can be a problem also.
> 
> There's no reason that the clock data has to be built into the kernel at
> all; we should support even SoC clock drivers as modules in an initrd.
> Alternatively, drop the unused data from the kernel after boot via
> __init or similar. Alternatively, "pre-link" the clock driver module
> into the kernel in a way that allows it to be unloaded after boot even
> though it was built-in.

Well, at least in case of all Samsung platforms, you need a functional 
clock driver for system boot-up, to initialize timers needed for 
scheduling (their frequencies are derived from rates of input clocks), 
ungate clocks of IP blocks and so on. This means that clocks must be 
available at early stage of kernel boot-up.

This doesn't imply, though, that clocks data will have to be built into 
the kernel in future. At the moment I don't think our driver model or 
initramfs handling is flexible enough to provide loadable modules with 
drivers that can be probed and possibly deferred at such early init. 
However, looking at future multiplatform kernels, it's hard to imagine 
using huge kernels packed with a lot of built-in drivers for every 
supported platform, so definitely a way to separate them from the kernel 
image will be needed.

> > You can just as easily claim that anything internal to SoC should be
> > left out from DT, as this is cast in stone (or rather, silicon) also.
> > We should only use it to describe board layout. Everything else, the
> > kernel can 'know' by compile time.
> 
> I did, immediately below:-) And then I went on to explain why that's
> necessary in many cases.
> 
> ...
> 
> > I can turn this around, as you went to this road. Why have DT at all?
> 
> I believe (at least for ARM) the idea was to avoid churn to the kernel
> for supporting the numerous different *boards*.
> 
> The kernel needs and contains drivers for HW blocks, and so since
> they're there, they may as well encode everything about the HW block.
> 
> However, in most cases, the kernel shouldn't contain drivers for boards;
> boards are built from various common components which have drivers. DT
> is used to describe how those components are inter-connected. Hence, we
> can hopefully remove all board-related churn from the kernel (once the
> DT files are moved out of the kernel).

I fully second this. That's why we have #interrupt-cells and one 
interrupt-controller node, instead of a bunch of single interrupt nodes. 
That's why we also have #gpio-cells and not nodes of single GPIO pins, 
although a shadow of the infamous idea of gpio- or pinctrl-simple is still 
visible, even here in this thread.

Moreover, if we look at this from a wider perspective, if we start to 
describe IP internals in device tree and make drivers rely on this, what 
happens when someone reuse the same IP or chip on an ACPI-driven x86 
system? (Intel already makes x86 based SoCs...)

If the driver had all the data about the IP inside, then ACPI, device tree 
or FEX^W any other hardware description method, even static platform 
drivers with static resources, could easily instantiate the driver, which 
would just work, regardless of the platform. Otherwise, the driver would 
need a glue retrieving data about the IP for every used description 
system. Is it something we are supposed to cope with?

> > Personally I hate the whole idea of a devicetree, however am forced to
> > use it as somebody decided it is a good idea to have one. It doesn't
> > really solve any problems, it just creates new ones in a way of
> > political BS where everybody claims they know how DT should be used,
> > and this just prevents people from actually using it at all. Also, it
> > creates just one new unnecessary dependency for boot, previously we
> > had bootloader and kernel images which had to be in sync, now we have
> > bootloader + DT + kernel. What next? Maybe we should move the clock
> > data into a firmware file of its own?
> 
> Well, I can sympathize, but I think the time is past for debating that.
> 
> > Why do you care so much what actually goes into the devicetree?

Well, why do we care so much what actually goes into the kernel? I believe 
both are the same reasons.

> To get DT right.
> 
> Even if we went back to board files and mach-xxx specific code rather
> than cleanly separated drivers, it would still be beneficial to have
> much more oversight of board/mach-xxx code than there was previously.
> Board files made it very easy to do SoC-specific hacks. To avoid that,
> in either DT or board files, we're trying to impose standards so that we
> pick correct, generic, appropriate solutions, rather than letting
> everyone run of with isolated ad-hoc solutions.

+1

> > Shouldn't people be let use it how they see fit?

This question can be easily reworded to: Shouldn't people be let to use 
whatever hacks they find enough for their own things to work?

> > For the clock
> > bindings
> > business this is the same, even if the bindings are there, you are
> > free
> > to use them if you like, and if you don't like them, you can do things
> > differently.
> 
> We'd be better of creating as much standardization as possible, so that
> all SoCs/boards/.. work as similarly as possible, and we achieve maximal
> code reuse, design-reuse, and allow people to understand everything
> rather than just one particular SoC's/board's solution.
> 
> If we don't get some re-use and standardization out of DT, we really may
> as well just use board files.

Well, I believe that board files could give us the same standardization 
level, but... _only_ if done correctly. The problem with board files was 
that they allowed many kinds of different hacks without really any level 
of control over contents of board files.

On the contrary, device tree enforces a lot of things and even if it takes 
some freedom from people which need to use it, it helps to keep things 
standardized.

Best regards,
Tomasz

Re: [PATCH v4 0/5] clk: dt: bindings for mux, divider & gate clocks

[Santosh Shilimkar]

Mike,

On Thursday 22 August 2013 01:53 AM, Mike Turquette wrote:
> This series introduces binding definitions for common register-mapped
> clock multiplexer, divider and gate IP blocks along with the
> corresponding setup functions for matching DT data.  The bindings are
> similar to the struct definitions but please don't hold that against the
> binding: the struct definitions closely model the hardware register
> layout.
> 
> The delta from v3 is very small and only consists of cosmetic changes
> and one functional change for better error handling.
> 
> Changes since v3:
> * added Tested-by & Reviewed-by from Heiko
> * replaced underscores with dashes in DT property names
> * bail from of clock setup function early if of_iomap fails
> * removed unecessary explict cast
> 
> Mike Turquette (5):
>   clk: divider: replace bitfield width with mask
>   clk: of: helper for determining number of parent clocks
>   clk: dt: binding for basic multiplexer clock
>   clk: dt: binding for basic divider clock
>   clk: dt: binding for basic gate clock
> 
I have used $subject series as the base for the Keystone
ccf support and tested these patches on Keystone EVM. Will
post those patches soon on the lists.

For the series if you need more tested-by tags, feel free to
add,
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH v4 0/5] clk: dt: bindings for mux, divider & gate clocks

[Tero Kristo]

On 08/29/2013 09:23 PM, Santosh Shilimkar wrote:
> Mike,
>
> On Thursday 22 August 2013 01:53 AM, Mike Turquette wrote:
>> This series introduces binding definitions for common register-mapped
>> clock multiplexer, divider and gate IP blocks along with the
>> corresponding setup functions for matching DT data.  The bindings are
>> similar to the struct definitions but please don't hold that against the
>> binding: the struct definitions closely model the hardware register
>> layout.
>>
>> The delta from v3 is very small and only consists of cosmetic changes
>> and one functional change for better error handling.
>>
>> Changes since v3:
>> * added Tested-by & Reviewed-by from Heiko
>> * replaced underscores with dashes in DT property names
>> * bail from of clock setup function early if of_iomap fails
>> * removed unecessary explict cast
>>
>> Mike Turquette (5):
>>    clk: divider: replace bitfield width with mask
>>    clk: of: helper for determining number of parent clocks
>>    clk: dt: binding for basic multiplexer clock
>>    clk: dt: binding for basic divider clock
>>    clk: dt: binding for basic gate clock
>>
> I have used $subject series as the base for the Keystone
> ccf support and tested these patches on Keystone EVM. Will
> post those patches soon on the lists.
>
> For the series if you need more tested-by tags, feel free to
> add,
> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>
>

Oh yea, same for my set I posted yesterday. Used this as basis for 
O3/O4/O5/DRA7/AM335x tests, so you can also add my Tested-by tag if you 
need one.

-Tero

Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock

[Haojian Zhuang]

On 22 August 2013 13:53, Mike Turquette <mturquette@linaro.org> wrote:
> Device Tree binding for the basic clock gate, plus the setup function to
> register the clock.  Based on the existing fixed-clock binding.
>
> A different approach to this was proposed in 2012[1] and a similar
> binding was proposed more recently[2] if anyone wants some extra
> reading.
>
> [1] http://article.gmane.org/gmane.linux.documentation/5679
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137878.html
>
> Tero Kristo contributed helpful bug fixes to this patch.
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Heiko Stuebner <heiko@sntech.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/gate-clock.txt       | 36 +++++++++++++++++
>  drivers/clk/clk-gate.c                             | 47 ++++++++++++++++++++++
>  include/linux/clk-provider.h                       |  2 +
>  3 files changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/gate-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/gate-clock.txt b/Documentation/devicetree/bindings/clock/gate-clock.txt
> new file mode 100644
> index 0000000..1c0d4d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/gate-clock.txt
> @@ -0,0 +1,36 @@
> +Binding for simple gate clock.
> +
> +This binding uses the common clock binding[1].  It assumes a
> +register-mapped clock gate controlled by a single bit that has only one
> +input clock or parent.  By default setting the specified bit gates the
> +clock signal and clearing the bit ungates it.
> +
> +The binding must provide the register to control the gate and the bit
> +shift for the corresponding gate control bit. Some clocks set the bit to
> +gate the clock signal, and clear it to ungate the clock signal. The
> +optional "set-bit-to-disable" specifies this behavior.
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "gate-clock".
> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : link to phandle of parent clock
> +- reg : base address for register controlling adjustable gate
> +- bit-shift : bit shift for programming the clock gate
> +
> +Optional properties:
> +- clock-output-names : from common clock binding.
> +- set-bit-to-disable : inverts default gate programming. Setting the bit
> +  gates the clock and clearing the bit ungates the clock.
> +- hiword-mask : lower half of the register controls the gate, upper half
> +  of the register indicates bits that were updated in the lower half
> +
> +Examples:
> +       clock_foo: clock_foo@4a008100 {
> +               compatible = "gate-clock";
> +               #clock-cells = <0>;
> +               clocks = <&clock_bar>;
> +               reg = <0x4a008100 0x4>
> +               bit-shift = <3>

There's some argument on my clock binding patch set of Hi3620.

I defined each clock as one clock node and some of them are sharing one
register. Stephen attacked on this since it means multiple clock node sharing
one register.

Now the same thing also exists in Mike's patch. Mike's patch could also
support this behavior. And it's very common that one register is sharing among
multiple clocks in every SoC. Which one should I follow?


> +       };
> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> index 2b28a00..63641c2 100644
> --- a/drivers/clk/clk-gate.c
> +++ b/drivers/clk/clk-gate.c
> @@ -15,6 +15,8 @@
>  #include <linux/io.h>
>  #include <linux/err.h>
>  #include <linux/string.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
>
>  /**
>   * DOC: basic gatable clock which can gate and ungate it's ouput
> @@ -162,3 +164,48 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
>         return clk;
>  }
>  EXPORT_SYMBOL_GPL(clk_register_gate);
> +
> +#ifdef CONFIG_OF
> +/**
> + * of_gate_clk_setup() - Setup function for simple gate rate clock
> + */
> +void of_gate_clk_setup(struct device_node *node)
> +{
> +       struct clk *clk;
> +       const char *clk_name = node->name;
> +       void __iomem *reg;
> +       const char *parent_name;
> +       u8 clk_gate_flags = 0;
> +       u32 bit_idx = 0;
> +
> +       of_property_read_string(node, "clock-output-names", &clk_name);
> +
> +       parent_name = of_clk_get_parent_name(node, 0);
> +
> +       reg = of_iomap(node, 0);

I suggest not using of_iomap for each clock node.

If each clock is one node, it means hundreds of clock node existing in
device tree. Hundreds of mapping page only cost unnecessary mapping.

Maybe we can resolve it by this way.

DTS file:
        clock register bank {
                reg = <{start} {size}>;
                #address-cells = <1>;
                #size-cells = <0>;             /* each clock only
exists in one register */

                clock node {
                        compatible = "xxx";
                        #clock-cells = <0>;
                        clock-output-names = yyy";
                        reg = <{offset}>;
                        ... other properties ...
                };
         };

clock driver:
         void __iomem *base, *reg;
         const __be32 *prop;
         /* base comes from the iomapping of the parent node */
         prop = of_get_address(np, 0, NULL, NULL);
         if (!prop)
               goto err;
         reg = base + be32_to_cpu(*prop);

Then we can get the register virtual address at here.

It could avoid always calling ioremap(). Maybe it's not the best
solution, it's just
for your reference.

> +       if (!reg) {
> +               pr_err("%s: no memory mapped for property reg\n", __func__);
> +               return;
> +       }
> +
> +       if (of_property_read_u32(node, "bit-shift", &bit_idx)) {
> +               pr_err("%s: missing bit-shift property for %s\n",
> +                               __func__, node->name);
> +               return;
> +       }
> +
> +       if (of_property_read_bool(node, "set-bit-to-disable"))
> +               clk_gate_flags |= CLK_GATE_SET_TO_DISABLE;
> +
> +       if (of_property_read_bool(node, "hiword-mask"))
> +               clk_gate_flags |= CLK_GATE_HIWORD_MASK;
> +
> +       clk = clk_register_gate(NULL, clk_name, parent_name, 0, reg, bit_idx,
> +                       clk_gate_flags, NULL);
> +
> +       if (!IS_ERR(clk))
> +               of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +}
> +EXPORT_SYMBOL_GPL(of_gate_clk_setup);
> +CLK_OF_DECLARE(gate_clk, "gate-clock", of_gate_clk_setup);
> +#endif
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 218d923..b471e37 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -240,6 +240,8 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
>                 void __iomem *reg, u8 bit_idx,
>                 u8 clk_gate_flags, spinlock_t *lock);
>
> +void of_gate_clk_setup(struct device_node *node);
> +
>  struct clk_div_table {
>         unsigned int    val;
>         unsigned int    div;
> --
> 1.8.1.2
>

Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock

[Stephen Warren]

On 08/29/2013 07:45 PM, Haojian Zhuang wrote:
> On 22 August 2013 13:53, Mike Turquette <mturquette@linaro.org> wrote:
>> Device Tree binding for the basic clock gate, plus the setup function to
>> register the clock.  Based on the existing fixed-clock binding.
>>
>> A different approach to this was proposed in 2012[1] and a similar
>> binding was proposed more recently[2] if anyone wants some extra
>> reading.

>> diff --git a/Documentation/devicetree/bindings/clock/gate-clock.txt b/Documentation/devicetree/bindings/clock/gate-clock.txt

>> +Binding for simple gate clock.
...
>> +       clock_foo: clock_foo@4a008100 {
>> +               compatible = "gate-clock";
>> +               #clock-cells = <0>;
>> +               clocks = <&clock_bar>;
>> +               reg = <0x4a008100 0x4>
>> +               bit-shift = <3>
> 
> There's some argument on my clock binding patch set of Hi3620.
> 
> I defined each clock as one clock node and some of them are sharing one
> register. Stephen attacked on this since it means multiple clock node sharing
> one register.

s/attacked/disagreed with/ I think:-)

> Now the same thing also exists in Mike's patch. Mike's patch could also
> support this behavior. And it's very common that one register is sharing among
> multiple clocks in every SoC. Which one should I follow?

I believe it's a matter of how the HW is structured.

If the HW truly has segregated register regions for each individual
clock, and is documented in a way that implies each individual clock is
a separate HW module, then it makes sense to describe each clock as a
separate DT node.

However, if the HW simply has a "clock module" that provides many
clocks, with inter-mingled registers all over the place, and is
documented as a single module that generates lots of clocks, then it
makes sense to describe that one module as a single DT node.

In other words, the DT representation should match the HW design and
documentation.

This is exactly why we have the #clock-cells property in DT; so that a
clock provider can provide multiple clocks if that's the way the HW is
designed.

>> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c

>> +void of_gate_clk_setup(struct device_node *node)
>> +{
>> +       struct clk *clk;
>> +       const char *clk_name = node->name;
>> +       void __iomem *reg;
>> +       const char *parent_name;
>> +       u8 clk_gate_flags = 0;
>> +       u32 bit_idx = 0;
>> +
>> +       of_property_read_string(node, "clock-output-names", &clk_name);
>> +
>> +       parent_name = of_clk_get_parent_name(node, 0);
>> +
>> +       reg = of_iomap(node, 0);
> 
> I suggest not using of_iomap for each clock node.
> 
> If each clock is one node, it means hundreds of clock node existing in
> device tree. Hundreds of mapping page only cost unnecessary mapping.

The page table entries will get re-used. I'm not familiar with the mm
code, but multiple of_iomap() for the exact same range probably just map
down to incrementing a refcount on some kernel data structure, so
actually has zero overhead?


> Maybe we can resolve it by this way.
> 
> DTS file:
>         clock register bank {

You need a compatible value here, in order to instantiate the top-level
driver for the "clock generator" HW block.

>                 reg = <{start} {size}>;
>                 #address-cells = <1>;
>                 #size-cells = <0>;             /* each clock only
> exists in one register */

You would need a ranges property here, to map the child reg properties
into the parent node's address space.

>                 clock node {
>                         compatible = "xxx";
>                         #clock-cells = <0>;
>                         clock-output-names = yyy";
>                         reg = <{offset}>;
>                         ... other properties ...
>                 };
>          };

That could be a reasonable solution; the existence of a single "clock
generator" HW block is clearly called out by the existing of the
top-level DT node, yet the internals of that node are free to be
whatever you want, since this is purely defined by the binding
definition for that top-level "clock generator" node.

That all said, I see almost zero value in having all these child nodes,
since the top-level compatible value implies every single detail about
the HW, so the list of clocks and their parameters could just as easily
be a table in the driver for the HW, in order to avoid having to parse
that data every boot just to end up with the same table...

The only exception would be if the SoC designer truly had composed the
top-level "clock generator" HW block out of many completely independent
re-usable clock IP blocks, and many SoCs existed that used those
individual clock blocks completely unchanged, without any SoC-specific
differences such as additional SoC-specific clock block types, so that
one could get greater re-use by parametrizing everything in DT. In my
(perhaps limited) experience of SoCS, this seems like an /extremely/
unlikely situation.

Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock

[Haojian Zhuang]

On 31 August 2013 04:06, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/29/2013 07:45 PM, Haojian Zhuang wrote:
>> On 22 August 2013 13:53, Mike Turquette <mturquette@linaro.org> wrote:
>>> Device Tree binding for the basic clock gate, plus the setup function to
>>> register the clock.  Based on the existing fixed-clock binding.
>>>
>>> A different approach to this was proposed in 2012[1] and a similar
>>> binding was proposed more recently[2] if anyone wants some extra
>>> reading.
>
>>> diff --git a/Documentation/devicetree/bindings/clock/gate-clock.txt b/Documentation/devicetree/bindings/clock/gate-clock.txt
>
>>> +Binding for simple gate clock.
> ...
>>> +       clock_foo: clock_foo@4a008100 {
>>> +               compatible = "gate-clock";
>>> +               #clock-cells = <0>;
>>> +               clocks = <&clock_bar>;
>>> +               reg = <0x4a008100 0x4>
>>> +               bit-shift = <3>
>>
>> There's some argument on my clock binding patch set of Hi3620.
>>
>> I defined each clock as one clock node and some of them are sharing one
>> register. Stephen attacked on this since it means multiple clock node sharing
>> one register.
>
> s/attacked/disagreed with/ I think:-)
>
>> Now the same thing also exists in Mike's patch. Mike's patch could also
>> support this behavior. And it's very common that one register is sharing among
>> multiple clocks in every SoC. Which one should I follow?
>
> I believe it's a matter of how the HW is structured.
>
> If the HW truly has segregated register regions for each individual
> clock, and is documented in a way that implies each individual clock is
> a separate HW module, then it makes sense to describe each clock as a
> separate DT node.
>
> However, if the HW simply has a "clock module" that provides many
> clocks, with inter-mingled registers all over the place, and is
> documented as a single module that generates lots of clocks, then it
> makes sense to describe that one module as a single DT node.
>
> In other words, the DT representation should match the HW design and
> documentation.
>
> This is exactly why we have the #clock-cells property in DT; so that a
> clock provider can provide multiple clocks if that's the way the HW is
> designed.
>
>>> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
>
>>> +void of_gate_clk_setup(struct device_node *node)
>>> +{
>>> +       struct clk *clk;
>>> +       const char *clk_name = node->name;
>>> +       void __iomem *reg;
>>> +       const char *parent_name;
>>> +       u8 clk_gate_flags = 0;
>>> +       u32 bit_idx = 0;
>>> +
>>> +       of_property_read_string(node, "clock-output-names", &clk_name);
>>> +
>>> +       parent_name = of_clk_get_parent_name(node, 0);
>>> +
>>> +       reg = of_iomap(node, 0);
>>
>> I suggest not using of_iomap for each clock node.
>>
>> If each clock is one node, it means hundreds of clock node existing in
>> device tree. Hundreds of mapping page only cost unnecessary mapping.
>
> The page table entries will get re-used. I'm not familiar with the mm
> code, but multiple of_iomap() for the exact same range probably just map
> down to incrementing a refcount on some kernel data structure, so
> actually has zero overhead?

I think it's not right. Let check the implemenation in ioremap().

__arm_ioremap_pfn_caller():

    /* try to reuse one of the static mapping whenever possible. */
    svm = find_static_vm_paddr();
    if (svm) {
         addr = svm->vm.addr;
         addr += paddr - svm->vm.phys_addr;
         return (void __iomem *)(offset + addr);
    }
    ...

    area = get_vm_area_caller(size, VM_IOREMAP, caller);
    addr = area->addr;
    ioremap_page_range(addr, addr+size, paddr, ..);

We can see that it'll try to find static mapping. What's the static mapping?
If we define iotable in machine driver, we have the static mapping, just like
debug_ll. If we parse everything from DTS file, it'll always get a new virtual
address from vm area. So it always create a new page mapping even for one
register.

>
>
>> Maybe we can resolve it by this way.
>>
>> DTS file:
>>         clock register bank {
>
> You need a compatible value here, in order to instantiate the top-level
> driver for the "clock generator" HW block.
>
>>                 reg = <{start} {size}>;
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;             /* each clock only
>> exists in one register */
>
> You would need a ranges property here, to map the child reg properties
> into the parent node's address space.
>
>>                 clock node {
>>                         compatible = "xxx";
>>                         #clock-cells = <0>;
>>                         clock-output-names = yyy";
>>                         reg = <{offset}>;
>>                         ... other properties ...
>>                 };
>>          };
>
> That could be a reasonable solution; the existence of a single "clock
> generator" HW block is clearly called out by the existing of the
> top-level DT node, yet the internals of that node are free to be
> whatever you want, since this is purely defined by the binding
> definition for that top-level "clock generator" node.
>
> That all said, I see almost zero value in having all these child nodes,
> since the top-level compatible value implies every single detail about
> the HW, so the list of clocks and their parameters could just as easily
> be a table in the driver for the HW, in order to avoid having to parse
> that data every boot just to end up with the same table...
>
> The only exception would be if the SoC designer truly had composed the
> top-level "clock generator" HW block out of many completely independent
> re-usable clock IP blocks, and many SoCs existed that used those
> individual clock blocks completely unchanged, without any SoC-specific
> differences such as additional SoC-specific clock block types, so that
> one could get greater re-use by parametrizing everything in DT. In my
> (perhaps limited) experience of SoCS, this seems like an /extremely/
> unlikely situation.
>

Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@linaro.org> [130903 20:11]:
> 
> We can see that it'll try to find static mapping. What's the static mapping?
> If we define iotable in machine driver, we have the static mapping, just like
> debug_ll. If we parse everything from DTS file, it'll always get a new virtual
> address from vm area. So it always create a new page mapping even for one
> register.

I may not follow you here.. But it seems that you've missing something with
the static mapping: It's found based on the physical address. So if you
create static mappings for your SoC with iotable_init(), those mappings
will be available everywhere including drivers when you do ioremap().

Regards,

Tony

Re: [PATCH v4 5/5] clk: dt: binding for basic gate clock

[Tomasz Figa]

Hi Tony,

On Wednesday 04 of September 2013 10:59:09 Tony Lindgren wrote:
> * Haojian Zhuang <haojian.zhuang@linaro.org> [130903 20:11]:
> > We can see that it'll try to find static mapping. What's the static
> > mapping? If we define iotable in machine driver, we have the static
> > mapping, just like debug_ll. If we parse everything from DTS file,
> > it'll always get a new virtual address from vm area. So it always
> > create a new page mapping even for one register.
> 
> I may not follow you here.. But it seems that you've missing something
> with the static mapping: It's found based on the physical address. So
> if you create static mappings for your SoC with iotable_init(), those
> mappings will be available everywhere including drivers when you do
> ioremap().

The thing is that today we are moving in favour of fully dynamic mapping, 
based on data from device tree, with as little as possible (or even no) 
static mapping based on hardcoded values.

So, back to the original problem, we end up doing multiple dynamic 
mappings of the same physical page, because there is no refcounting in 
ioremap and, if it doesn't find a static mapping containing the region 
we're interested in, it simply creates a new mapping.

Best regards,
Tomasz