Re: [PATCHv9 08/43] clk: ti: add composite clock support
From: Nishanth Menon <nm@ti.com>
Date: 2013-10-31 16:27:08
Also in:
linux-arm-kernel, linux-omap
On 10/25/2013 10:57 AM, Tero Kristo wrote:
quoted hunk ↗ jump to hunk
This is a multipurpose clock node, which contains support for multiple sub-clocks. Uses basic composite clock type to implement the actual functionality, and TI specific gate, mux and divider clocks. Signed-off-by: Tero Kristo <redacted> --- .../devicetree/bindings/clock/ti/composite.txt | 54 +++++ drivers/clk/ti/Makefile | 2 +- drivers/clk/ti/composite.c | 222 ++++++++++++++++++++ include/linux/clk/ti.h | 8 + 4 files changed, 285 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/composite.txt create mode 100644 drivers/clk/ti/composite.cdiff --git a/Documentation/devicetree/bindings/clock/ti/composite.txt b/Documentation/devicetree/bindings/clock/ti/composite.txt new file mode 100644 index 0000000..5f43c47 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/composite.txt@@ -0,0 +1,54 @@ +Binding for TI composite clock. + +Binding status: Unstable - ABI compatibility may be broken in the future + +This binding uses the common clock binding[1]. It assumes a +register-mapped composite clock with multiple different sub-types; + +a multiplexer clock with multiple input clock signals or parents, one +of which can be selected as output, this behaves exactly as [2] + +an adjustable clock rate divider, this behaves exactly as [3] + +a gating function which can be used to enable and disable the output +clock, this behaves exactly as [4] + +The binding must provide a list of the component clocks that shall be +merged to this clock. The component clocks shall be of one of the +"ti,*composite*-clock" types. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt +[2] Documentation/devicetree/bindings/clock/ti/mux.txt +[3] Documentation/devicetree/bindings/clock/ti/divider.txt +[4] Documentation/devicetree/bindings/clock/ti/gate.txt + +Required properties: +- compatible : shall be: "ti,composite-clock" +- clocks : link phandles of component clocks +- #clock-cells : from common clock binding; shall be set to 0. + +Examples: + +usb_l4_gate_ick: usb_l4_gate_ick { + #clock-cells = <0>; + compatible = "ti,composite-interface-clock"; + clocks = <&l4_ick>; + ti,bit-shift = <5>; + reg = <0x0a10>; +}; + +usb_l4_div_ick: usb_l4_div_ick { + #clock-cells = <0>; + compatible = "ti,composite-divider-clock"; + clocks = <&l4_ick>; + ti,bit-shift = <4>; + ti,max-div = <1>; + reg = <0x0a40>; + ti,index-starts-at-one; +}; + +usb_l4_ick: usb_l4_ick { + #clock-cells = <0>; + compatible = "ti,composite-clock"; + clocks = <&usb_l4_gate_ick>, <&usb_l4_div_ick>; +};diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile index 533efb4..a4a7595 100644 --- a/drivers/clk/ti/Makefile +++ b/drivers/clk/ti/Makefile@@ -1,3 +1,3 @@ ifneq ($(CONFIG_OF),) -obj-y += clk.o dpll.o autoidle.o +obj-y += clk.o dpll.o autoidle.o composite.o endifdiff --git a/drivers/clk/ti/composite.c b/drivers/clk/ti/composite.c new file mode 100644 index 0000000..9ce7e54 --- /dev/null +++ b/drivers/clk/ti/composite.c@@ -0,0 +1,222 @@ +/* + * TI composite clock support + * + * Copyright (C) 2013 Texas Instruments, Inc. + * + * Tero Kristo <t-kristo@ti.com> + * + * 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 + * published by the Free Software Foundation. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk-provider.h> +#include <linux/slab.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/clk/ti.h> +#include <linux/list.h> + +#define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw) + +static unsigned long ti_composite_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + return clk_divider_ops.recalc_rate(hw, parent_rate); +} + +static long ti_composite_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *prate) +{ + return -EINVAL; +} + +static int ti_composite_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + return -EINVAL; +} + +static const struct clk_ops ti_composite_divider_ops = { + .recalc_rate = &ti_composite_recalc_rate, + .round_rate = &ti_composite_round_rate, + .set_rate = &ti_composite_set_rate, +}; + +static const struct clk_ops ti_composite_gate_ops = { + .enable = &omap2_dflt_clk_enable, + .disable = &omap2_dflt_clk_disable, + .is_enabled = &omap2_dflt_clk_is_enabled, +}; + +struct component_clk { + int num_parents; + const char **parent_names; + struct device_node *node; + int type; + struct clk_hw *hw; + struct list_head link; +}; + +static const char * __initdata component_clk_types[] = {
[CLK_COMPONENT_TYPE_MAX]?
+ "mux", "gate", "divider",
+};
+
+static LIST_HEAD(component_clks);
+
+static struct component_clk *_lookup_component(struct device_node *node, int i)
+{
+ int rc;
+ struct of_phandle_args clkspec;
+ struct component_clk *comp;
+
+ rc = of_parse_phandle_with_args(node, "clocks", "#clock-cells", i,
+ &clkspec);
+ if (rc)
+ return NULL;
+
+ list_for_each_entry(comp, &component_clks, link) {
+ if (comp->node == clkspec.np)
+ return comp;
+ }
+ return NULL;
+}
+
+static inline struct clk_hw *_get_hw(struct component_clk *clk)
+{
+ if (clk)
+ return clk->hw;
+
+ return NULL;
+}
+
+static int __init of_ti_composite_clk_setup(struct device_node *node,
+ struct regmap *regmap)
+{
+ int num_clks;
+ int i;
+ struct clk *clk;
+ struct component_clk *comp;
+ struct component_clk *clks[3] = { NULL };sizeof(component_clk_types) or CLK_COMPONENT_TYPE_MAX instead of 3?
+ const char *name = node->name;
+ int num_parents = 0;
+ const char **parent_names = NULL;
+ int ret = 0;
+
+ /* Number of component clocks to be put inside this clock */
+ num_clks = of_clk_get_parent_count(node);
+
+ if (num_clks < 1) {
+ pr_err("%s: ti composite clk %s must have component(s)\n",
+ __func__, name);using pr_fmt will reduce the pr_err code.
+ return -EINVAL;
+ }
+
+ /* Check for presence of each component clock */
+ for (i = 0; i < num_clks; i++) {
+ comp = _lookup_component(node, i);
+ if (!comp)
+ return -EAGAIN;
+ if (clks[comp->type] != NULL) {
+ pr_err("%s: duplicate component types for %s (%s)!\n",
+ __func__, name, component_clk_types[comp->type]);
+ return -EINVAL;would you not want to do a cleanup of the list here?
+ }
+ clks[comp->type] = comp;
+ }
+
+ /* All components exists, proceed with registration */
+ for (i = 0; i < 3; i++) {CLK_COMPONENT_TYPE_MAX instead
+ if (!clks[i])
+ continue;
+ if (clks[i]->num_parents) {
+ num_parents = clks[i]->num_parents;
+ parent_names = clks[i]->parent_names;
+ }
+ }
+
+ /* Enforce mux parents if exists */
+ comp = clks[CLK_COMPONENT_TYPE_MUX];
+ if (comp) {
+ num_parents = comp->num_parents;
+ parent_names = comp->parent_names;
+ }you could move the for loop in the else case and reduce the loops to CLK_COMPONENT_TYPE_MUX+1 to CLK_COMPONENT_TYPE_MAX.
+
+ if (!num_parents) {
+ pr_err("%s: no parents found for %s!\n", __func__,
+ name);
+ return -EINVAL;would you not want to do a cleanup of the list here?
+ }
+
+ clk = clk_register_composite(NULL, name, parent_names, num_parents,
+ _get_hw(clks[CLK_COMPONENT_TYPE_MUX]),
+ &clk_mux_ops,
+ _get_hw(clks[CLK_COMPONENT_TYPE_DIVIDER]),
+ &ti_composite_divider_ops,
+ _get_hw(clks[CLK_COMPONENT_TYPE_GATE]),
+ &ti_composite_gate_ops, 0);
+
+ if (!IS_ERR(clk)) {
+ of_clk_add_provider(node, of_clk_src_simple_get, clk);
+ goto cleanup;
+ }
+
+ ret = PTR_ERR(clk);
+cleanup:
+ /* Free component clock list entries */
+ for (i = 0; i < 3; i++) {
+ if (!clks[i])
+ continue;
+ list_del(&clks[i]->link);
+ kfree(clks[i]);
+ }could you not just do a kfree(clks[i]) and set the component_clks to NULL? Further, if there are only 3 clocks that can ever be present and we do not allow for duplicates types, could we not do those checks in ti_clk_add_component instead of having a harder recovery in setup of composite clk?
quoted hunk ↗ jump to hunk
+ + return ret; +} +CLK_OF_DECLARE(ti_composite_clock, "ti,composite-clock", + of_ti_composite_clk_setup); + +int __init ti_clk_add_component(struct device_node *node, struct clk_hw *hw, + int type) +{ + int num_parents; + const char **parent_names; + struct component_clk *clk; + int i; + + num_parents = of_clk_get_parent_count(node); + + if (num_parents < 1) { + pr_err("%s: component-clock %s must have parent(s)\n", __func__, + node->name); + return -EINVAL; + } + + parent_names = kzalloc((sizeof(char *) * num_parents), GFP_KERNEL); + if (!parent_names) + return -ENOMEM; + + for (i = 0; i < num_parents; i++) + parent_names[i] = of_clk_get_parent_name(node, i); + + clk = kzalloc(sizeof(*clk), GFP_KERNEL); + if (!clk) { + kfree(parent_names); + return -ENOMEM; + } + + clk->num_parents = num_parents; + clk->parent_names = parent_names; + clk->hw = hw; + clk->node = node; + clk->type = type; + list_add(&clk->link, &component_clks); + + return 0; +}diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h index e45005c..96c9725 100644 --- a/include/linux/clk/ti.h +++ b/include/linux/clk/ti.h@@ -137,6 +137,13 @@ struct clk_hw_omap { /* DPLL Type and DCO Selection Flags */ #define DPLL_J_TYPE 0x1 +/* Composite clock component types */ +enum { + CLK_COMPONENT_TYPE_MUX = 0, + CLK_COMPONENT_TYPE_GATE, + CLK_COMPONENT_TYPE_DIVIDER
adding a CLK_COMPONENT_TYPE_MAX will allow a better implementation
quoted hunk ↗ jump to hunk
+}; + /** * struct ti_dt_clk - OMAP DT clock alias declarations * @lk: clock lookup definition@@ -179,6 +186,7 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate, void ti_dt_clocks_register(struct ti_dt_clk *oclks); void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap); int of_ti_autoidle_setup(struct device_node *node, struct regmap *regmap); +int ti_clk_add_component(struct device_node *node, struct clk_hw *hw, int type); #ifdef CONFIG_OF void of_ti_clk_allow_autoidle_all(void);
-- Regards, Nishanth Menon