From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tero Kristo Subject: Re: [PATCHv9 09/43] CLK: ti: add support for ti divider-clock Date: Fri, 1 Nov 2013 11:48:47 +0200 Message-ID: <527378FF.6020905@ti.com> References: <1382716658-6964-1-git-send-email-t-kristo@ti.com> <1382716658-6964-10-git-send-email-t-kristo@ti.com> <52729B50.5060503@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52729B50.5060503@ti.com> Sender: linux-omap-owner@vger.kernel.org To: Nishanth Menon , linux-omap@vger.kernel.org, paul@pwsan.com, tony@atomide.com, bcousson@baylibre.com, rnayak@ti.com, mturquette@linaro.org Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org List-Id: devicetree@vger.kernel.org On 10/31/2013 08:02 PM, Nishanth Menon wrote: > On 10/25/2013 10:57 AM, Tero Kristo wrote: >> This patch adds support for TI divider clock binding, which simply uses >> the basic clock divider to provide the features needed. >> >> Signed-off-by: Tero Kristo >> --- >> .../devicetree/bindings/clock/ti/divider.txt | 86 +++++++ >> drivers/clk/ti/Makefile | 3 +- >> drivers/clk/ti/divider.c | 239 ++++++++++++++++++++ >> 3 files changed, 327 insertions(+), 1 deletion(-) >> create mode 100644 Documentation/devicetree/bindings/clock/ti/divider.txt >> create mode 100644 drivers/clk/ti/divider.c >> >> diff --git a/Documentation/devicetree/bindings/clock/ti/divider.txt b/Documentation/devicetree/bindings/clock/ti/divider.txt >> new file mode 100644 >> index 0000000..65e3dcd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/ti/divider.txt >> @@ -0,0 +1,86 @@ >> +Binding for TI divider 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 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: >> + >> +ti,index-starts-at-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 >> + >> +ti,index-power-of-two - valid divisor values are powers of two. E.g: >> +register value actual divisor value >> +0 1 >> +1 2 >> +2 4 >> + >> +Additionally an array of valid dividers may be supplied like so: >> + >> + dividers = <4>, <8>, <0>, <16>; > ti,dividers I believe. True. > >> + >> +Which will map the resulting values to a divisor table by their index: >> +register value actual divisor value >> +0 4 >> +1 8 >> +2 >> +3 16 >> + >> +Any zero value in this array means the corresponding bit-value is invalid >> +and must not be used. >> + >> +The binding must also provide the register to control the divider and >> +unless the divider array is provided, min and max dividers. 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. >> + >> +Required properties: >> +- compatible : shall be "ti,divider-clock". > > ti,composite-divider-clock undocumented? Hmm yea true. > >> +- #clock-cells : from common clock binding; shall be set to 0. >> +- clocks : link to phandle of parent clock >> +- reg : offset for register controlling adjustable divider >> + >> +Optional properties: >> +- clock-output-names : from common clock binding. >> +- ti,dividers : array of integers defining divisors >> +- ti,bit-shift : number of bits to shift the divider value, defaults to 0 >> +- ti,min-div : min divisor for dividing the input clock rate, only >> + needed if the first divisor is offset from the default value (1) >> +- ti,max-div : max divisor for dividing the input clock rate, only needed >> + if ti,dividers is not defined. >> +- ti,index-starts-at-one : valid divisor programming starts at 1, not zero > > makes sense only if ti,dividers are not defined, right? > CLK_DIVIDER_ONE_BASED is used with !table Yeah, ignored if table is present. > >> +- ti,index-power-of-two : valid divisor programming must be a power of two > > makes sense only if ti,dividers are not defined, right? > CLK_DIVIDER_POWER_OF_TWO is used with !table Yea. > >> +- ti,autoidle-shift : bit shift of the autoidle enable bit for the clock >> +- ti,invert-autoidle-bit : autoidle is enabled by setting the bit to 0 > > These are part of auto idle driver bindings, so maybe give a link to that? Yea can do. > >> +- ti,set-rate-parent : clk_set_rate is propagated to parent > > yeah - this is one of those properties that should probably become > generic at a later point in time. > >> + >> +Examples: >> +dpll_usb_m2_ck: dpll_usb_m2_ck@4a008190 { >> + #clock-cells = <0>; >> + compatible = "ti,divider-clock"; >> + clocks = <&dpll_usb_ck>; >> + ti,max-div = <127>; >> + reg = <0x190>; >> + ti,index-starts-at-one; >> +}; >> + >> +aess_fclk: aess_fclk@4a004528 { >> + #clock-cells = <0>; >> + compatible = "ti,divider-clock"; >> + clocks = <&abe_clk>; >> + ti,bit-shift = <24>; >> + reg = <0x528>; >> + ti,max-div = <2>; >> +}; > > an example of ti,dividers will be useful as well. Ok. > >> diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile >> index a4a7595..640ebf9 100644 >> --- a/drivers/clk/ti/Makefile >> +++ b/drivers/clk/ti/Makefile >> @@ -1,3 +1,4 @@ >> ifneq ($(CONFIG_OF),) >> -obj-y += clk.o dpll.o autoidle.o composite.o >> +obj-y += clk.o dpll.o autoidle.o divider.o \ >> + composite.o >> endif >> diff --git a/drivers/clk/ti/divider.c b/drivers/clk/ti/divider.c >> new file mode 100644 >> index 0000000..787bc8f >> --- /dev/null >> +++ b/drivers/clk/ti/divider.c >> @@ -0,0 +1,239 @@ >> +/* >> + * TI Divider Clock >> + * >> + * Copyright (C) 2013 Texas Instruments, Inc. >> + * >> + * Tero Kristo >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +static struct clk_div_table >> +__init *ti_clk_get_div_table(struct device_node *node) >> +{ >> + struct clk_div_table *table; >> + const __be32 *divspec; >> + u32 val; >> + u32 num_div; >> + u32 valid_div; >> + int i; >> + >> + divspec = of_get_property(node, "ti,dividers", &num_div); >> + >> + if (!divspec) >> + return NULL; >> + >> + num_div /= 4; >> + >> + valid_div = 0; >> + >> + /* Determine required size for divider table */ >> + for (i = 0; i < num_div; i++) { >> + of_property_read_u32_index(node, "ti,dividers", i, &val); >> + if (val) >> + valid_div++; >> + } >> + >> + if (!valid_div) { >> + pr_err("%s: no valid dividers for %s table\n", __func__, >> + node->name); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + table = kzalloc(sizeof(*table) * (valid_div + 1), GFP_KERNEL); >> + >> + if (!table) { >> + pr_err("%s: unable to allocate memory for %s table\n", __func__, >> + node->name); > > you could drop this. True. > >> + return ERR_PTR(-ENOMEM); >> + } >> + >> + valid_div = 0; >> + >> + for (i = 0; i < num_div; i++) { >> + of_property_read_u32_index(node, "ti,dividers", i, &val); >> + if (val) { >> + table[valid_div].div = val; >> + table[valid_div].val = i; >> + valid_div++; >> + } >> + } >> + >> + return table; >> +} >> + >> +static int _get_divider_width(struct device_node *node, >> + const struct clk_div_table *table, >> + u8 flags) >> +{ >> + u32 min_div; >> + u32 max_div; >> + u32 val = 0; >> + u32 div; >> + >> + if (!table) { >> + /* Clk divider table not provided, determine min/max divs */ >> + if (of_property_read_u32(node, "ti,min-div", &min_div)) { >> + pr_debug("%s: no min-div for %s, default=1\n", >> + __func__, node->name); >> + min_div = 1; >> + } >> + >> + if (of_property_read_u32(node, "ti,max-div", &max_div)) { >> + pr_err("%s: no max-div for %s!\n", __func__, >> + node->name); >> + return -EINVAL; > > will be nice to get warning if ti,divider is defined and any of the > non-operational properties are defined as well. Hmm ok, can add checks for those. > >> + } >> + >> + /* Determine bit width for the field */ >> + if (flags & CLK_DIVIDER_ONE_BASED) >> + val = 1; >> + >> + div = min_div; >> + >> + while (div < max_div) { >> + if (flags & CLK_DIVIDER_POWER_OF_TWO) >> + div <<= 1; >> + else >> + div++; >> + val++; >> + } >> + } else { >> + div = 0; >> + >> + while (table[div].div) { >> + val = table[div].val; >> + div++; >> + } >> + } >> + >> + return fls(val); >> +} >> + >> +/** >> + * of_ti_divider_clk_setup() - Setup function for simple div rate clock >> + */ >> +static int __init of_ti_divider_clk_setup(struct device_node *node, >> + struct regmap *regmap) >> +{ >> + struct clk *clk; >> + const char *clk_name = node->name; >> + void __iomem *reg; >> + const char *parent_name; >> + u8 clk_divider_flags = 0; >> + u8 width = 0; >> + u8 shift = 0; >> + struct clk_div_table *table = NULL; >> + u32 val = 0; >> + u32 flags = 0; >> + int ret = 0; >> + >> + parent_name = of_clk_get_parent_name(node, 0); >> + >> + if (of_property_read_u32(node, "reg", &val)) { >> + pr_err("%s: %s must have reg\n", __func__, clk_name); >> + return -EINVAL; >> + } >> + >> + reg = (void *)val; >> + >> + if (!of_property_read_u32(node, "ti,bit-shift", &val)) >> + shift = val; >> + >> + if (of_property_read_bool(node, "ti,index-starts-at-one")) >> + clk_divider_flags |= CLK_DIVIDER_ONE_BASED; >> + >> + if (of_property_read_bool(node, "ti,index-power-of-two")) >> + clk_divider_flags |= CLK_DIVIDER_POWER_OF_TWO; >> + >> + if (of_property_read_bool(node, "ti,set-rate-parent")) >> + flags |= CLK_SET_RATE_PARENT; >> + >> + table = ti_clk_get_div_table(node); >> + >> + if (IS_ERR(table)) >> + return PTR_ERR(table); >> + >> + ret = _get_divider_width(node, table, clk_divider_flags); >> + if (ret < 0) >> + goto cleanup; >> + >> + width = ret; >> + >> + clk = clk_register_divider_table_regmap(NULL, clk_name, parent_name, >> + flags, reg, regmap, shift, >> + width, clk_divider_flags, table, >> + NULL); >> + >> + if (!IS_ERR(clk)) { >> + of_clk_add_provider(node, of_clk_src_simple_get, clk); >> + ret = of_ti_autoidle_setup(node, regmap); > > if this fails, table will be freed though, we have added provider and > registerd table_regmap, no? Provider and regmap are shared for the whole IP block (CM/PRM whatever.) Those are only initialized once. > >> + } else { >> + ret = PTR_ERR(clk); >> + } >> + >> + if (!ret) >> + return 0; >> +cleanup: >> + kfree(table); >> + return ret; >> +} >> +CLK_OF_DECLARE(divider_clk, "ti,divider-clock", of_ti_divider_clk_setup); >> + >> +static int __init of_ti_composite_divider_clk_setup(struct device_node *node, >> + struct regmap *regmap) >> +{ >> + struct clk_divider *div; >> + u32 val; >> + int ret; >> + >> + div = kzalloc(sizeof(*div), GFP_KERNEL); >> + if (!div) >> + return -ENOMEM; >> + >> + of_property_read_u32(node, "reg", &val); > > reg is a mandatory property, no? error checks? Ok can add. :P > >> + div->reg = (void *)val; >> + div->regmap = regmap; >> + >> + div->table = ti_clk_get_div_table(node); >> + >> + if (of_property_read_bool(node, "ti,index-starts-at-one")) >> + div->flags |= CLK_DIVIDER_ONE_BASED; > > alright, this does not really map back to ti,divder-clock style > handling.. aren't they supposed to be consistent? > how about "ti,index-power-of-two", bit shift etc? I can add non-used property checks yes. >> + >> + ret = _get_divider_width(node, div->table, div->flags); >> + if (ret < 0) >> + goto cleanup; >> + >> + div->width = ret; >> + >> + if (of_property_read_u32(node, "ti,bit-shift", &val)) { >> + pr_debug("%s: missing bit-shift property for %s, default=0\n", >> + __func__, node->name); >> + val = 0; >> + } >> + div->shift = val; >> + >> + ret = ti_clk_add_component(node, &div->hw, CLK_COMPONENT_TYPE_DIVIDER); >> + if (!ret) >> + return 0; >> + >> +cleanup: >> + kfree(div); >> + return ret; >> +} >> +CLK_OF_DECLARE(ti_composite_divider_clk, "ti,composite-divider-clock", >> + of_ti_composite_divider_clk_setup); >> > >