clk: dt: bindings for mux & divider clocks
Subsystems (2)
cover clk: dt: bindings for mux & divider clocks
This series introduces binding definitions for common register-mapped clock multiplexor and divider IP blocks, and the corresponding setup functions once they are matched. The bindings are close the struct definitions but please don't hold that against the binding: the struct definitions closely model the hardware.
The only missing basic clock type is the gate clock. A binding for that was posted some time back and is similar in spirit to these[1]. I guess we'll need to decide whether register-level programming details belong in DT. I believe they do since those details describe the hardware.
Note that there is still no generic clock driver that matches these basic types, but it would be trivial to write one. Thoughts on that? Is it better for each of the basic clock types to be a driver that matches, or should there be one drivers/clk/clk-basic.c which matches all of the basic clock building blocks? I like the latter for aesthetic purposes.
I am using this code while converting the OMAP4 clock data over to DT and some common boilerplate code can be factored out of several clock drivers if this is merged.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137878.html
Mike Turquette (3):
clk: of: helper for determining number of parent clocks
clk: dt: binding for basic multiplexor clock
clk: dt: binding for basic divider clock
.../devicetree/bindings/clock/divider-clock.txt | 80 ++++++++++++++++++++ .../devicetree/bindings/clock/mux-clock.txt | 75 ++++++++++++++++++ drivers/clk/clk-divider.c | 88 +++++++++++++++++++++- drivers/clk/clk-mux.c | 65 +++++++++++++++- drivers/clk/clk.c | 6 ++ include/linux/clk-provider.h | 8 +- 6 files changed, 319 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/divider-clock.txt create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
Signature · 1 line
1.8.1.2
RFC PATCH 1/3 clk: of: helper for determining number of parent clocks
Walks the "clocks" array of parent clock phandles and returns the number.
---
DT gurus, please let me know if this is stupid. I am unfamiliar with parsing DT blobs and associated APIs.
drivers/clk/clk.c | 6 ++++++ include/linux/clk-provider.h | 1 + 2 files changed, 7 insertions(+)
drivers/clk/clk.c +6 -0
@@ -2082,6 +2082,12 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)return clk; } +int of_clk_get_parent_count (struct device_node *np) +{ +return of_count_phandle_with_args (np,"clocks" ,"#clock-cells" ); +} +EXPORT_SYMBOL_GPL (of_clk_get_parent_count); +const char *of_clk_get_parent_name (struct device_node *np,int index) {struct of_phandle_args clkspec;
include/linux/clk-provider.h +1 -0
@@ -442,6 +442,7 @@ void of_clk_del_provider(struct device_node *np);struct clk *of_clk_src_simple_get (struct of_phandle_args *clkspec,void *data);struct clk *of_clk_src_onecell_get (struct of_phandle_args *clkspec,void *data); +int of_clk_get_parent_count (struct device_node *np);const char *of_clk_get_parent_name (struct device_node *np,int index);void of_clk_init (const struct of_device_id *matches);
Signature · 1 line
1.8.1.2
RFC PATCH 2/3 clk: dt: binding for basic multiplexor clock
In reply to Mike TurquetteDevice Tree binding for the basic clock multiplexor, plus the setup function to register the clock. Based on the existing fixed-clock binding.
Also relocate declaration of of_fixed_factor_clk_setup to keep things tidy.
--- .../devicetree/bindings/clock/mux-clock.txt | 75 ++++++++++++++++++++++ drivers/clk/clk-mux.c | 65 ++++++++++++++++++- include/linux/clk-provider.h | 5 +- 3 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
Documentation/devicetree/bindings/clock/mux-clock.txt new file +75 -0
@@ -0,0 +1,75 @@ +Binding for simple mux clock. + +This binding uses the common clock binding[1]. It assumes a +register-mapped multiplexor 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_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>; + +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 +- 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 +- shift : number of bits to shift the mask, defaults to 0 if not present +- index_one : valid input select programming starts at 1, not zero + +Examples: + clock: clock at 4a008100 { + compatible = "mux-clock"; + #clock-cells = <0>; + clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>; + reg = <0x4a008100 0x4> + mask = <0x3>; + index_one; + }; + + clock: clock at 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>; + };
drivers/clk/clk-mux.c +64 -1
80 lines not on this page · raw message
include/linux/clk-provider.h +4 -1
@@ -325,7 +325,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
@@ -346,10 +346,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 *
Signature · 1 line
1.8.1.2
Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock
Hi Mike,
I think it's a multiplexEr clock in the patch title, and see below
Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
Quoted 8 lines
Device Tree binding for the basic clock multiplexor, plus the setup function to register the clock. Based on the existing fixed-clock binding.
Also relocate declaration of
of_fixed_factor_clk_setupto keep things tidy.Signed-off-by Mike Turquette <[email protected]>
[...]
Quoted 8 lines
drivers/clk/clk-mux.c +8
-0+ + reg =of_iomap (node,0 ); +pr_err ("%s: reg is 0x%p\n" , __func__, reg); + +if (of_property_read_u32 (node,"mask" , &mask)) { +pr_err ("%s: missing mask property for %s\n" , __func__, node->name); +return ; + }
drivers/clk/clk-mux.c struct clk *clk_register_mux(struct device *dev, const char *name, · line 210
Quoted 10 lines
drivers/clk/clk-mux.c +10
-0+ +if (of_property_read_u32 (node,"shift" , &shift)) +pr_debug ("%s: missing shift property defaults to zero for %s\n" , + __func__, node->name); + +if (of_property_read_bool (node,"index_one" )) + clk_mux_flags |=CLK_MUX_INDEX_ONE ; + + clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents, + 0, reg, 0, mask, clk_mux_flags,
^- should probably be shift
Otherwise looks cool and I'm currently trying it with my Rockchip code.
Heiko
Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock
Quoting Heiko St?bner (2013-06-03 12:33:19)
Quoted 4 lines
Hi Mike,
I think it's a multiplexEr clock in the patch title, and see below
Doh, you are right. But "xor" is so much cooler looking than "xer"...
Quoted 35 lines
Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
Quoted 8 lines
Device Tree binding for the basic clock multiplexor, plus the setup function to register the clock. Based on the existing fixed-clock binding.
Also relocate declaration of
of_fixed_factor_clk_setupto keep things tidy.Signed-off-by Mike Turquette <[email protected]>[...]
Quoted 8 lines
drivers/clk/clk-mux.c +8
-0+ + reg =of_iomap (node,0 ); +pr_err ("%s: reg is 0x%p\n" , __func__, reg); + +if (of_property_read_u32 (node,"mask" , &mask)) { +pr_err ("%s: missing mask property for %s\n" , __func__, node->name); +return ; + }drivers/clk/clk-mux.c struct clk *clk_register_mux(struct device *dev, const char *name, · line 210
Quoted 10 lines
drivers/clk/clk-mux.c +10
-0+ +if (of_property_read_u32 (node,"shift" , &shift)) +pr_debug ("%s: missing shift property defaults to zero for %s\n" , + __func__, node->name); + +if (of_property_read_bool (node,"index_one" )) + clk_mux_flags |=CLK_MUX_INDEX_ONE ; + + clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents, + 0, reg, 0, mask, clk_mux_flags,^- should probably be shift
Quoted 2 lines
Otherwise looks cool and I'm currently trying it with my Rockchip code.
Right again. My test platform seems to not shift the mask at all so this did not cause a visible bug for me.
Thanks for the review,
Mike
Quoted 2 lines
Heiko
Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock
Am Montag, 3. Juni 2013, 22:07:22 schrieb Mike Turquette:
Quoted 41 lines
Quoting Heiko St?bner (2013-06-03 12:33:19)
Quoted 3 lines
Hi Mike,
I think it's a multiplexEr clock in the patch title, and see below
Doh, you are right. But "xor" is so much cooler looking than "xer"...
Quoted 33 lines
Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
Quoted 8 lines
Device Tree binding for the basic clock multiplexor, plus the setup function to register the clock. Based on the existing fixed-clock binding.
Also relocate declaration of
of_fixed_factor_clk_setupto keep things tidy.Signed-off-by Mike Turquette <[email protected]>[...]
Quoted 17 lines
drivers/clk/clk-mux.c +6
-0+ + reg =of_iomap (node,0 ); +pr_err ("%s: reg is 0x%p\n" , __func__, reg); + +if (of_property_read_u32 (node,"mask" , &mask)) { + pr_err("%s: missing mask property for %s\n", __func__,
node->name); + return;drivers/clk/clk-mux.c +4
-0+ } + +if (of_property_read_u32 (node,"shift" , &shift)) + pr_debug("%s: missing shift property defaults to zero for%s\n", +
__func__,node->name);drivers/clk/clk-mux.c +5
-0+ +if (of_property_read_bool (node,"index_one" )) + clk_mux_flags |=CLK_MUX_INDEX_ONE ; + + clk = clk_register_mux_table(NULL, clk_name, parent_names,Quoted 2 lines
num_parents, + 0, reg, 0, mask,clk_mux_flags,^- should probably be shift
Quoted 4 lines
Quoted 1 line
Otherwise looks cool and I'm currently trying it with my Rockchip code.
Right again. My test platform seems to not shift the mask at all so this did not cause a visible bug for me.
I'm currently converting my rockchip stuff to it, as it would solve a lot of problems with the numerous clock types I'm defining just to put the shift and mask values somewhere.
So lets see if anything more turns up ;-)
Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock
Am Montag, 3. Juni 2013, 22:15:45 schrieb Heiko St?bner:
Quoted 44 lines
Am Montag, 3. Juni 2013, 22:07:22 schrieb Mike Turquette:
Quoted 43 lines
Quoting Heiko St?bner (2013-06-03 12:33:19)
Quoted 3 lines
Hi Mike,
I think it's a multiplexEr clock in the patch title, and see below
Doh, you are right. But "xor" is so much cooler looking than "xer"...
Quoted 34 lines
Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
Quoted 8 lines
Device Tree binding for the basic clock multiplexor, plus the setup function to register the clock. Based on the existing fixed-clock binding.
Also relocate declaration of
of_fixed_factor_clk_setupto keep things tidy.Signed-off-by Mike Turquette <[email protected]>[...]
Quoted 17 lines
drivers/clk/clk-mux.c +6
-0+ + reg =of_iomap (node,0 ); +pr_err ("%s: reg is 0x%p\n" , __func__, reg); + +if (of_property_read_u32 (node,"mask" , &mask)) { + pr_err("%s: missing mask property for %s\n", __func__,
node->name); + return;drivers/clk/clk-mux.c +4
-0+ } + +if (of_property_read_u32 (node,"shift" , &shift)) + pr_debug("%s: missing shift property defaults to zerofor %s\n", +
__func__,node->name);drivers/clk/clk-mux.c +5
-0+ +if (of_property_read_bool (node,"index_one" )) + clk_mux_flags |=CLK_MUX_INDEX_ONE ; + + clk = clk_register_mux_table(NULL, clk_name, parent_names,Quoted 2 lines
num_parents, + 0, reg, 0, mask,clk_mux_flags,^- should probably be shift
Otherwise looks cool and I'm currently trying it with my Rockchip code.
Quoted 8 lines
Quoted 2 lines
Right again. My test platform seems to not shift the mask at all so this did not cause a visible bug for me.
I'm currently converting my rockchip stuff to it, as it would solve a lot of problems with the numerous clock types I'm defining just to put the shift and mask values somewhere.
So lets see if anything more turns up ;-)
yep ... of_mux_clk_setup is quite talkative ... there are quite some (debug-?) pr_errs in it ;-)
Heiko
Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock
Quoted 48 lines · Heiko St?bner
Am Montag, 3. Juni 2013, 22:15:45 schrieb Heiko St?bner:
Quoted 47 lines
Am Montag, 3. Juni 2013, 22:07:22 schrieb Mike Turquette:
Quoted 45 lines
Quoting Heiko St?bner (2013-06-03 12:33:19)
Quoted 3 lines
Hi Mike,
I think it's a multiplexEr clock in the patch title, and see below
Doh, you are right. But "xor" is so much cooler looking than "xer"...
Quoted 33 lines
Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
Quoted 8 lines
Device Tree binding for the basic clock multiplexor, plus the setup function to register the clock. Based on the existing fixed-clock binding.
Also relocate declaration of
of_fixed_factor_clk_setupto keep things tidy.Signed-off-by Mike Turquette <[email protected]>[...]
Quoted 17 lines
drivers/clk/clk-mux.c +6
-0+ + reg =of_iomap (node,0 ); +pr_err ("%s: reg is 0x%p\n" , __func__, reg); + +if (of_property_read_u32 (node,"mask" , &mask)) { + pr_err("%s: missing mask property for %s\n", __func__,
node->name); + return;drivers/clk/clk-mux.c +4
-0+ } + +if (of_property_read_u32 (node,"shift" , &shift)) + pr_debug("%s: missing shift property defaults to zerofor %s\n", +
__func__,node->name);drivers/clk/clk-mux.c +5
-0+ +if (of_property_read_bool (node,"index_one" )) + clk_mux_flags |=CLK_MUX_INDEX_ONE ; + + clk = clk_register_mux_table(NULL, clk_name, parent_names,Quoted 2 lines
num_parents, + 0, reg, 0, mask,clk_mux_flags,^- should probably be shift
Quoted 1 line
Otherwise looks cool and I'm currently trying it with my Rockchip code.
Right again. My test platform seems to not shift the mask at all so this did not cause a visible bug for me.
Quoted 9 lines
Quoted 5 lines
I'm currently converting my rockchip stuff to it, as it would solve a lot of problems with the numerous clock types I'm defining just to put the shift and mask values somewhere.
So lets see if anything more turns up ;-)
yep ...
of_mux_clk_setupis quite talkative ... there are quite some (debug-?)pr_errsin it ;-)
Yes I realized that after sending. Hopefully not too many curse words in the trace messages. Will remove for v2.
Regards,
Mike
Quoted 2 lines
Heiko
RFC PATCH 3/3 clk: dt: binding for basic divider clock
In reply to Mike TurquetteDevicetree binding for the basic clock divider, plus the setup function to register the clock. Based on the existing fixed-clock binding.
--- .../devicetree/bindings/clock/divider-clock.txt | 80 ++++++++++++++++++++ drivers/clk/clk-divider.c | 88 +++++++++++++++++++++- include/linux/clk-provider.h | 2 + 3 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/divider-clock.txt
Documentation/devicetree/bindings/clock/divider-clock.txt new file +80 -0
@@ -0,0 +1,80 @@ +Binding for simple divider clock. + +This binding uses the common clock binding[1]. It assumes a +register-mapped adjustable clock rate divider that does not gate and has +only one input clock or parent. By default the value programmed into +the register is one less than the actual divisor value. E.g: + +register value actual divisor value +0 1 +1 2 +2 3 + +This assumption may be modified by the following optional properties: + +index_one - valid divisor values start at 1, not the default of 0. E.g: +register value actual divisor value +1 1 +2 2 +3 3 + +index_power_of_two - valid divisor values are powers of two. E.g: +register value actual divisor value +0 1 +1 2 +2 4 + +allow_zero - same as index_one, but zero is divide-by-1. E.g: +register value actual divisor value +0 1 +1 1 +2 2 + +Additionally a table of valid dividers may be supplied like so: + + table = <4 0>, <8, 1>; + +where the first value in the pair is the divider and the second value is +the programmed register bitfield. + +The binding must also provide the register to control the divider 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 "divider-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 divider +- mask : arbitrary bitmask for programming the adjustable divider + +Optional properties: +- clock-output-names : From common clock binding. +- table : array of integer pairs defining divisors & bitfield values +- shift : number of bits to shift the mask, defaults to 0 if not present +- index_one : valid divisor programming starts at 1, not zero +- index_power_of_two : valid divisor programming must be a power of two +- allow_zero : implies index_one, and programming zero results in + divide-by-one + +Examples: + clock_foo: clock_foo at 4a008100 { + compatible = "divider-clock"; + #clock-cells = <0>; + clocks = <&clock_baz>; + reg = <0x4a008100 0x4> + mask = <0x3> + }; + + clock_bar: clock_bar at 4a008108 { + #clock-cells = <0>; + compatible = "divider-clock"; + clocks = <&clock_foo>; + reg = <0x4a008108 0x4>; + mask = <0x1>; + shift = <0>; + table = < 4 0 >, < 8 1 >; + };
drivers/clk/clk-divider.c +87 -1
103 lines not on this page · raw message
include/linux/clk-provider.h +2 -0
@@ -283,6 +283,8 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name, u8 clk_divider_flags, const struct clk_div_table *table, spinlock_t *lock); +void of_divider_clk_setup (struct device_node *node); +/** * struct clk_mux - multiplexer clock *
Signature · 1 line
1.8.1.2
Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock
Am Montag, 3. Juni 2013, 19:53:10 schrieb Mike Turquette:
Quoted 5 lines
Devicetree binding for the basic clock divider, plus the setup function to register the clock. Based on the existing fixed-clock binding.
Signed-off-by Mike Turquette <[email protected]>---
[...]
drivers/clk/clk-divider.c struct clk *clk_register_divider_table(struct device *dev, const char *name, · line 366
Quoted 12 lines
drivers/clk/clk-divider.c +12
-0+/** +* of_div_clk_setup() - Setup function for simple div rate clock +*/ +void of_divider_clk_setup (struct device_node *node) +{ +struct clk *clk; +const char *clk_name = node->name; +void __iomem *reg; +const char *parent_name; +u8 clk_divider_flags =0 ; +u8 mask =0 ; +u8 shift =0 ;
in the mux-clock these 3 are unsigned long and u32 types ... what is correct?
Quoted 6 lines
drivers/clk/clk-divider.c +6
-0+struct clk_div_table *table; + +of_property_read_string (node,"clock-output-names" , &clk_name); + + parent_name =of_clk_get_parent_name (node,0 ); +
drivers/clk/clk-divider.c struct clk *clk_register_divider_table(struct device *dev, const char *name, · line 382
Quoted 10 lines
drivers/clk/clk-divider.c +10
-0+ reg =of_iomap (node,0 ); + +if (of_property_read_u8 (node,"mask" , &mask)) { +pr_err ("%s: missing mask property for %s\n" , __func__, node->name); +return ; + } + +if (of_property_read_u8 (node,"shift" , &shift)) +pr_debug ("%s: missing shift property defaults to zero for %s\n" , + __func__, node->name);
same here ... mux reads u32
Quoted 8 lines
drivers/clk/clk-divider.c +8
-0+if (of_property_read_bool (node,"index_one" )) + clk_divider_flags |=CLK_DIVIDER_ONE_BASED ; + +if (of_property_read_bool (node,"index_power_of_two" )) + clk_divider_flags |=CLK_DIVIDER_POWER_OF_TWO ; + +if (of_property_read_bool (node,"index_allow_zero" )) + clk_divider_flags |=CLK_DIVIDER_ALLOW_ZERO ;
drivers/clk/clk-divider.c struct clk *clk_register_divider_table(struct device *dev, const char *name, · line 401
Quoted 10 lines
drivers/clk/clk-divider.c +10
-0+ + table =of_clk_get_div_table (node); +if (IS_ERR (table)) +return ; + + clk =clk_register_divider_table (NULL , clk_name, + parent_name,0 , + reg, shift, mask, + clk_divider_flags, table, +NULL );
this causes trouble, as the divider clock code above still requires a width instead of a mask. I remember talk about this going to change separately, but couldn't find anything of the sort in linux-next.
Quoted 7 lines
drivers/clk/clk-divider.c +7
-0+ +if (!IS_ERR (clk)) + of_clk_add_provider(node, of_clk_src_simple_get, clk); +} +EXPORT_SYMBOL_GPL (of_divider_clk_setup); +CLK_OF_DECLARE (divider_clk,"divider-clock" , of_divider_clk_setup); +#endif
Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock
Quoting Heiko St?bner (2013-06-03 15:18:32)
Quoted 9 lines
Am Montag, 3. Juni 2013, 19:53:10 schrieb Mike Turquette:
Quoted 5 lines
Devicetree binding for the basic clock divider, plus the setup function to register the clock. Based on the existing fixed-clock binding.
Signed-off-by Mike Turquette <[email protected]>---
[...]
Quoted 15 lines
drivers/clk/clk-divider.c struct clk *clk_register_divider_table(struct device *dev, const char *name, · line 366
Quoted 12 lines
drivers/clk/clk-divider.c +12
-0+/** +* of_div_clk_setup() - Setup function for simple div rate clock +*/ +void of_divider_clk_setup (struct device_node *node) +{ +struct clk *clk; +const char *clk_name = node->name; +void __iomem *reg; +const char *parent_name; +u8 clk_divider_flags =0 ; +u8 mask =0 ; +u8 shift =0 ;in the mux-clock these 3 are unsigned long and u32 types ... what is correct?
Good catch. Shifts should be u8, masks should be u32. I've left the flags as u8. The binding doesn't dictate this and if we end up having more flags (hopefully not!) then this value can be embiggened.
Quoted 34 lines
Quoted 7 lines
drivers/clk/clk-divider.c +6
-0+struct clk_div_table *table; + +of_property_read_string (node,"clock-output-names" , &clk_name); + + parent_name =of_clk_get_parent_name (node,0 ); +drivers/clk/clk-divider.c struct clk *clk_register_divider_table(struct device *dev, const char *name, · line 382
Quoted 10 lines
drivers/clk/clk-divider.c +10
-0+ reg =of_iomap (node,0 ); + +if (of_property_read_u8 (node,"mask" , &mask)) { +pr_err ("%s: missing mask property for %s\n" , __func__, node->name); +return ; + } + +if (of_property_read_u8 (node,"shift" , &shift)) +pr_debug ("%s: missing shift property defaults to zero for %s\n" , + __func__, node->name);same here ... mux reads u32
Quoted 14 lines
drivers/clk/clk-divider.c +14
-0+if (of_property_read_bool (node,"index_one" )) + clk_divider_flags |=CLK_DIVIDER_ONE_BASED ; + +if (of_property_read_bool (node,"index_power_of_two" )) + clk_divider_flags |=CLK_DIVIDER_POWER_OF_TWO ; + +if (of_property_read_bool (node,"index_allow_zero" )) + clk_divider_flags |=CLK_DIVIDER_ALLOW_ZERO ; + + table =of_clk_get_div_table (node); +if (IS_ERR (table)) +return ; + + clk = clk_register_divider_table(NULL, clk_name,
Quoted 8 lines
drivers/clk/clk-divider.c struct clk *clk_register_divider_table(struct device *dev, const char *name, · line 401
Quoted 4 lines
drivers/clk/clk-divider.c +4
-0+ parent_name,0 , + reg, shift, mask, + clk_divider_flags, table, + NULL);this causes trouble, as the divider clock code above still requires a width instead of a mask. I remember talk about this going to change separately, but couldn't find anything of the sort in linux-next.
Right. I viewed creation of the DT bindings a bit separately from the CCF code, which I think is the right approach. A mask is definitely a more useful structure than a width value, and the DT bindings need to be at least a little future-proof, so I chose a mask.
I'll update the divider code to use a mask and post that as part of the v2 series.
Regards,
Mike
Quoted 10 lines
Quoted 10 lines
drivers/clk/clk-divider.c +7
-0+ +if (!IS_ERR (clk)) + of_clk_add_provider(node, of_clk_src_simple_get, clk); +} +EXPORT_SYMBOL_GPL (of_divider_clk_setup); +CLK_OF_DECLARE (divider_clk,"divider-clock" , of_divider_clk_setup); +#endif
Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock
In reply to Mike TurquetteQuoted 5 lines · Mike Turquette
Documentation/devicetree/bindings/clock/divider-clock.txt new file +5
-0+Required properties: +- compatible : shall be "divider-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 divider
Documentation/devicetree/bindings/clock/divider-clock.txt line 61
Quoted 10 lines
Documentation/devicetree/bindings/clock/divider-clock.txt new file +10
-0+- mask : arbitrary bitmask for programming the adjustable divider + +Optional properties: +- clock-output-names : From common clock binding. +- table : array of integer pairs defining divisors & bitfield values +- shift : number of bits to shift the mask, defaults to 0 if not present +- index_one : valid divisor programming starts at 1, not zero +- index_power_of_two : valid divisor programming must be a power of two +- allow_zero : implies index_one, and programming zero results in + divide-by-one
It's preferred that property names have dashes instead of underscores.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock
Quoted 10 lines · Stephen Boyd
Quoted 9 lines · Mike Turquette
Documentation/devicetree/bindings/clock/divider-clock.txt new file +9
-0+Required properties: +- compatible : shall be "divider-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 divider +- mask : arbitrary bitmask for programming the adjustable divider + +Optional properties: +- clock-output-names : From common clock binding.
Quoted 8 lines
Quoted 6 lines
+- table : array of integer pairs defining divisors & bitfield values +- shift : number of bits to shift the mask, defaults to 0 if not present +- index_one : valid divisor programming starts at 1, not zero +- index_power_of_two : valid divisor programming must be a power of two +- allow_zero : implies index_one, and programming zero results in + divide-by-oneIt's preferred that property names have dashes instead of underscores.
I think I have a suggestion or two here..
I mailed one to Mike earlier, I have had some mailing list problems which I think are solved now..
If you provide a mask for programming the divider, surely the "shift" property is therefore redundant. Unless some device has a completely strange way of doing things and uses two seperated bitfields for programming in the same register, the shift value is simply a function of ffs() on the mask, isn't it? It is nice to put the information specifically in the device tree, but where a shift is not specified it'd probably be a good idea to go about deriving that value that way anyway, right, and if we're doing that.. why specify it?
Also it may be much simpler to simply define the default clock setup as being an integer divider that follows the form 2^n - I am not sure I can think off the top of my head of any clock dividers doing the rounds that have divide-by-non-power-of-two and if they exist out there (please correct me) they would be the rare ones. I think properties that modify behavior should err on the side of being rarely used and to define unusual behavior, not confirm the status quo.
Just to be sure, though, someone would need to go visit a bunch of current clock handlers already implemented or read a lot of docs to see how they're expecting their dividers. Maybe some of the Samsung, Qualcomm, TI, Freescale guys can comment on what is the most common case inside their SoCs.
In any case, once you figure out that, rather than specifying a full table of <0 1>, <1 2>, <2, 4> and creating a big list, you could just give the lower and upper limits and let the driver figure out what is in between with two values. If there were gaps, a full table would be necessary. The allow zero property might be better served by the very same range property.
I would also say.. bit-mask rather than mask, divider-table rather than table, divider-range (modifying the idea above). While we're inside a particular namespace in the binding I think naming properties with their intent (i.e. what table is it?) is good behavior.
--
Matt Sealey <[email protected]>
Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock
Quoting Matt Sealey (2013-06-04 10:39:53)
Quoted 25 lines · Stephen Boyd
Quoted 10 lines · Mike Turquette
Quoted 9 lines
Documentation/devicetree/bindings/clock/divider-clock.txt new file +9
-0+Required properties: +- compatible : shall be "divider-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 divider +- mask : arbitrary bitmask for programming the adjustable divider + +Optional properties: +- clock-output-names : From common clock binding.Quoted 8 lines
Quoted 6 lines
+- table : array of integer pairs defining divisors & bitfield values +- shift : number of bits to shift the mask, defaults to 0 if not present +- index_one : valid divisor programming starts at 1, not zero +- index_power_of_two : valid divisor programming must be a power of two +- allow_zero : implies index_one, and programming zero results in + divide-by-oneIt's preferred that property names have dashes instead of underscores.
I think I have a suggestion or two here..
I mailed one to Mike earlier, I have had some mailing list problems which I think are solved now..
Quoted 9 lines
If you provide a mask for programming the divider, surely the "shift" property is therefore redundant. Unless some device has a completely strange way of doing things and uses two seperated bitfields for programming in the same register, the shift value is simply a function of
ffs()on the mask, isn't it? It is nice to put the information specifically in the device tree, but where a shift is not specified it'd probably be a good idea to go about deriving that value that way anyway, right, and if we're doing that.. why specify it?
Shift is optional. Also I think the integer values in a DTS are 32-bits, so if some day someone had a clock driver that needed to write to a 64-bit register then shift might be useful there, combined with a 32-bit mask. Or perhaps dtc will have a 64-bit value for that case? I don't know.
I certainly do see your point about only using the mask, and the original clock basic types did this a while back before the shift+width style came into vogue.
If other reviewers also want to use a pure 32-bit mask and no shift then I'll drop it in the next round.
Quoted 8 lines
Also it may be much simpler to simply define the default clock setup as being an integer divider that follows the form 2^n - I am not sure I can think off the top of my head of any clock dividers doing the rounds that have divide-by-non-power-of-two and if they exist out there (please correct me) they would be the rare ones. I think properties that modify behavior should err on the side of being rarely used and to define unusual behavior, not confirm the status quo.
Please review the current use of CLK_DIVIDER_POWER_OF_TWO in the kernel. The only platform using this flag for power-of-two dividers is OMAP2430. Based on usage of clk_register_divider I think the common case is a clock divider that divides by any integer value programmed into it, up to the limit of the bitfield width.
Perhaps your platform is the corner case, in which case power-of-two is the property you should use.
Quoted 6 lines
Just to be sure, though, someone would need to go visit a bunch of current clock handlers already implemented or read a lot of docs to see how they're expecting their dividers. Maybe some of the Samsung, Qualcomm, TI, Freescale guys can comment on what is the most common case inside their SoCs.
The following platforms use the basic divider without the power-of-two flag:
imx6, loongson1B, mmp2, pxa168, pxa190, spearX, sunxi, tegra2/3/114, omap2+.
Quoted 7 lines
In any case, once you figure out that, rather than specifying a full table of <0 1>, <1 2>, <2, 4> and creating a big list, you could just give the lower and upper limits and let the driver figure out what is in between with two values. If there were gaps, a full table would be necessary. The allow zero property might be better served by the very same range property.
You have described how it already works. The table is only there for when a common formula fails to capture the possible divisor combinations. For the default the driver starts the index at 0 and goes up to mask/width, for index_one the driver does the same but starts at 1, not 0, for power of two ... well you get the picture. The table is only of use when these common divider schemes are insufficient to determine the divisors.
Quoted 5 lines
I would also say.. bit-mask rather than mask, divider-table rather than table, divider-range (modifying the idea above). While we're inside a particular namespace in the binding I think naming properties with their intent (i.e. what table is it?) is good behavior.
I'll make the properties more verbose in the next patch.
Thanks for the review,
Mike
Quoted 2 lines
--
Matt Sealey <[email protected]>
Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock
Quoted 41 lines · Mike Turquette
Quoting Matt Sealey (2013-06-04 10:39:53)
Quoted 25 lines · Stephen Boyd
Quoted 10 lines · Mike Turquette
Quoted 9 lines
Documentation/devicetree/bindings/clock/divider-clock.txt new file +9
-0+Required properties: +- compatible : shall be "divider-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 divider +- mask : arbitrary bitmask for programming the adjustable divider + +Optional properties: +- clock-output-names : From common clock binding.Quoted 8 lines
Quoted 6 lines
+- table : array of integer pairs defining divisors & bitfield values +- shift : number of bits to shift the mask, defaults to 0 if not present +- index_one : valid divisor programming starts at 1, not zero +- index_power_of_two : valid divisor programming must be a power of two +- allow_zero : implies index_one, and programming zero results in + divide-by-oneIt's preferred that property names have dashes instead of underscores.
I think I have a suggestion or two here..
I mailed one to Mike earlier, I have had some mailing list problems which I think are solved now..
Quoted 8 lines
If you provide a mask for programming the divider, surely the "shift" property is therefore redundant. Unless some device has a completely strange way of doing things and uses two seperated bitfields for programming in the same register, the shift value is simply a function of
ffs()on the mask, isn't it? It is nice to put the information specifically in the device tree, but where a shift is not specified it'd probably be a good idea to go about deriving that value that way anyway, right, and if we're doing that.. why specify it?Shift is optional. Also I think the integer values in a DTS are 32-bits, so if some day someone had a clock driver that needed to write to a 64-bit register then shift might be useful there, combined with a 32-bit mask. Or perhaps dtc will have a 64-bit value for that case? I don't know.
Quoted 6 lines
I certainly do see your point about only using the mask, and the original clock basic types did this a while back before the shift+width style came into vogue.
If other reviewers also want to use a pure 32-bit mask and no shift then I'll drop it in the next round.
Hmm.. that's a very good point...
You can't have a 64-bit OF property unless you're on a 64-bit platform or do some really weird workarounds for it (multiple cells etc.). That's how the OF spec was laid out, it sucks.. but it's the standard and the DT seems to stay compatible with the general concept. A supplemental binding to support clock dividers living in 64-bit registers may be a better way.
You'd need a shift for the mask if you could only do a 32-bit property (32-bit platform with a 4-byte cell size) and a shift for the bits that would go into the mask as well as a width property for the register size to bound the shift.. Too complicated for now. I would say stick with 32-bit until this magical 64-bit divider register appears, and then do something slightly different :)
Quoted 9 lines
Quoted 7 lines
Also it may be much simpler to simply define the default clock setup as being an integer divider that follows the form 2^n - I am not sure I can think off the top of my head of any clock dividers doing the rounds that have divide-by-non-power-of-two and if they exist out there (please correct me) they would be the rare ones. I think properties that modify behavior should err on the side of being rarely used and to define unusual behavior, not confirm the status quo.
Please review the current use of
CLK_DIVIDER_POWER_OF_TWOin the kernel.
You're right, I think I got it backwards or was actually thinking of allow_zero (as in, 0x0 divider = divide by 1)? i.MX definitely has more 0x0=div-by-1 than any other kind..
Quoted 7 lines
Quoted 6 lines
In any case, once you figure out that, rather than specifying a full table of <0 1>, <1 2>, <2, 4> and creating a big list, you could just give the lower and upper limits and let the driver figure out what is in between with two values. If there were gaps, a full table would be necessary. The allow zero property might be better served by the very same range property.
Quoted 7 lines
You have described how it already works. The table is only there for when a common formula fails to capture the possible divisor combinations. For the default the driver starts the index at 0 and goes up to mask/width, for
index_onethe driver does the same but starts at 1, not 0, for power of two ... well you get the picture. The table is only of use when these common divider schemes are insufficient to determine the divisors.
Right, but in the case of index_one and allow_zero, one property for ranges basically covers the case where a slight modification to the formula used would be required, and allows specifying the first valid divider and the last valid divider which might come in handy when you have a mask of a certain size (say, 6 bits), but the actual divider is only 4 bits wide. You may need to ensure that you clear reserved bits within that masked region, but not use the mask itself to calculate the full range.
I can't think of a good case but on i.MX53 the DVFSP load tracking register 1 has a field that subdivides the ARM core clock by 1, 2 or 3 (using 0, 1 or 2 as the divider value) in a 6-bit field, where 000011-111111 are reserved. I would think the correct value for the mask is the full 6-bit field, but the range should define that only 1-3 are valid (saving one entry). It isn't really something that would end up using a clock framework divider though.
If there's a divider built for future expandability or somehow restricted at the design level to only accept a few bits, or only 'restricted' at the documentation level (.. it's been done) then it might be best to use the mask to mask off the register, and not to define valid bits to set, unless you resolve to specify two masks.. but a range property would be better and cover every case, while not having to force DT writers to list every divider in a big field (imagine if it's 3 bits that's a 16 entry table.. there are some 5 and 6-bit clock dividers floating around), I think an opportunity to consolidate two properties into one AND give an easy out to oddly-ranged dividers (where low and high divider settings may be 'invalid' but between those values is a contiguous range of a significant number of values (and here I would say, more than 2).
Since we shouldn't be guessing, divider-p-o-2 is still required but as you corrected me.. rarer.
Quoted 7 lines
Quoted 5 lines
I would also say.. bit-mask rather than mask, divider-table rather than table, divider-range (modifying the idea above). While we're inside a particular namespace in the binding I think naming properties with their intent (i.e. what table is it?) is good behavior.
I'll make the properties more verbose in the next patch.
Lovely :)
Signature · 1 line
Matt Sealey <[email protected]>
Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock
In reply to Mike TurquetteAm Dienstag, 4. Juni 2013, 21:22:43 schrieb Mike Turquette:
Quoted 3 lines
Quoting Matt Sealey (2013-06-04 10:39:53)
Quoted 1 line
On Tue, Jun 4, 2013 at 12:11 PM, Stephen Boyd <[email protected]>
Quoted 39 lines · wrote:
Quoted 24 lines
Quoted 10 lines · Mike Turquette
Quoted 9 lines
Documentation/devicetree/bindings/clock/divider-clock.txt new file +9
-0+Required properties: +- compatible : shall be "divider-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 divider +- mask : arbitrary bitmask for programming the adjustable divider + +Optional properties: +- clock-output-names : From common clock binding.Quoted 8 lines
Quoted 6 lines
+- table : array of integer pairs defining divisors & bitfield values +- shift : number of bits to shift the mask, defaults to 0 if notpresent +-
index_one: valid divisor programming starts at 1, not zero +-index_power_of_two: valid divisor programming must be a power of two +-allow_zero: impliesindex_one, and programming zero results in + divide-by-oneIt's preferred that property names have dashes instead of underscores.
I think I have a suggestion or two here..
I mailed one to Mike earlier, I have had some mailing list problems which I think are solved now..
Quoted 8 lines
If you provide a mask for programming the divider, surely the "shift" property is therefore redundant. Unless some device has a completely strange way of doing things and uses two seperated bitfields for programming in the same register, the shift value is simply a function of
ffs()on the mask, isn't it? It is nice to put the information specifically in the device tree, but where a shift is not specified it'd probably be a good idea to go about deriving that value that way anyway, right, and if we're doing that.. why specify it?Shift is optional. Also I think the integer values in a DTS are 32-bits, so if some day someone had a clock driver that needed to write to a 64-bit register then shift might be useful there, combined with a 32-bit mask. Or perhaps dtc will have a 64-bit value for that case? I don't know.
Quoted 6 lines
I certainly do see your point about only using the mask, and the original clock basic types did this a while back before the shift+width style came into vogue.
If other reviewers also want to use a pure 32-bit mask and no shift then I'll drop it in the next round.
I would object to dropping the shift :-)
If you look at the two clk extensions in my rockchip patches you can see that they use a different regiment to set values.
Using the lower 16 bits to set the value and the upper 16 bit to mark which values are changed, i.e. (mask << shift + 16) | (val << shift).
(I'm not sure, if the naming or solution could be improved though)
Heiko
Am Montag, 3. Juni 2013, 19:53:07 schrieb Mike Turquette:
Quoted 18 lines
This series introduces binding definitions for common register-mapped clock multiplexor and divider IP blocks, and the corresponding setup functions once they are matched. The bindings are close the struct definitions but please don't hold that against the binding: the struct definitions closely model the hardware.
The only missing basic clock type is the gate clock. A binding for that was posted some time back and is similar in spirit to these[1]. I guess we'll need to decide whether register-level programming details belong in DT. I believe they do since those details describe the hardware.
Note that there is still no generic clock driver that matches these basic types, but it would be trivial to write one. Thoughts on that? Is it better for each of the basic clock types to be a driver that matches, or should there be one
drivers/clk/clk-basic.cwhich matches all of the basic clock building blocks? I like the latter for aesthetic purposes.
Quoted 3 lines
I am using this code while converting the OMAP4 clock data over to DT and some common boilerplate code can be factored out of several clock drivers if this is merged.
apart from the stuff pointed out in the replies to the patches this works really well on my upcoming Rockchip platform and saves quite a lot silly clock definitions whose only purpose is to hold the shift and width values.
So, for this series:
Mike,
Quoted 1 line · Mike Turquette
I am using this code while converting the OMAP4 clock data over to DT
I see these basic clk bindings can be useful for platforms that have a relatively simple clock tree, but I'm a little surprised that you plan to move OMAP4 to it. I'm wondering how many nodes you will have to add to OMAP4 device tree. For imx6q example, it means ~200 nodes addition to device tree, which is obviously a bloating of device tree.
Shawn
Quoted 2 lines
and some common boilerplate code can be factored out of several clock drivers if this is merged.
cover clk: dt: bindings for mux & divider clocks
Quoting Shawn Guo (2013-06-06 22:51:31)
Quoted 11 lines
Mike,
Quoted 1 line · Mike Turquette
I am using this code while converting the OMAP4 clock data over to DT
I see these basic clk bindings can be useful for platforms that have a relatively simple clock tree, but I'm a little surprised that you plan to move OMAP4 to it. I'm wondering how many nodes you will have to add to OMAP4 device tree. For imx6q example, it means ~200 nodes addition to device tree, which is obviously a bloating of device tree.
Yes, there was a time when I was firmly against doing such a thing. However I'm not sure it is so bad now. More and more SoCs are going to keep getting merged into the kernel and that just means more and more clock data. Perhaps DT is best suited to handle this bloat. I broke the clock data out into a separate file so that it didn't overwhelm the existing omap4.dtsi.
Either way I marked this series as RFC precisely due to your point. I want feedback on how the OMAP folks feel about this. So far no has has NACKed it ;-)
Regards,
Mike
Quoted 4 lines
Shawn
Quoted 2 lines
and some common boilerplate code can be factored out of several clock drivers if this is merged.
Quoted 6 lines · Mike Turquette
Yes, there was a time when I was firmly against doing such a thing. However I'm not sure it is so bad now. More and more SoCs are going to keep getting merged into the kernel and that just means more and more clock data. Perhaps DT is best suited to handle this bloat. I broke the clock data out into a separate file so that it didn't overwhelm the existing
omap4.dtsi.
I'm actually more concerned by run-time impact. Any of_find_node_*() call will possibly have to scan all those hundreds of nodes to find the desired one. Anyway, it's an OMAP folks decision, and the impact might not be that much on those fast CPUs.
Shawn
Quoted 4 lines
Either way I marked this series as RFC precisely due to your point. I want feedback on how the OMAP folks feel about this. So far no has has NACKed it ;-)
cover clk: dt: bindings for mux & divider clocks
Quoting Shawn Guo (2013-06-07 20:02:41)
Quoted 8 lines · Mike Turquette
Quoted 6 lines
Yes, there was a time when I was firmly against doing such a thing. However I'm not sure it is so bad now. More and more SoCs are going to keep getting merged into the kernel and that just means more and more clock data. Perhaps DT is best suited to handle this bloat. I broke the clock data out into a separate file so that it didn't overwhelm the existing
omap4.dtsi.
Quoted 5 lines
I'm actually more concerned by run-time impact. Any of_find_node_*() call will possibly have to scan all those hundreds of nodes to find the desired one. Anyway, it's an OMAP folks decision, and the impact might not be that much on those fast CPUs.
Agreed. Once the conversion is complete we'll have to measure and see if any negligible boot-time latency is present. Alternatively we could avoid registering the whole clock framework at boot time and instead register clocks as needed (either by loading the omap4 clock driver module or even on a per-device basis). Tony has hinted at this a few times.
Regards,
Mike
Quoted 6 lines
Shawn
Quoted 4 lines
Either way I marked this series as RFC precisely due to your point. I want feedback on how the OMAP folks feel about this. So far no has has NACKed it ;-)