Re: [PATCHv9 00/43] ARM: TI SoC clock DT conversion
On 10/31/2013 08:55 AM, Nishanth Menon wrote:
> On 10/31/2013 04:10 AM, Tero Kristo wrote:
>> On 10/30/2013 10:10 PM, Nishanth Menon wrote:
>>> On 10/30/2013 10:00 AM, Nishanth Menon wrote:
On 10/30/2013 03:23 AM, Tero Kristo wrote:
> On 10/29/2013 06:19 PM, Nishanth Menon wrote:
>> On 10/25/2013 10:56 AM, Tero Kristo wrote:
>>
>>> Testing done:
>>> - omap3-beagle: boot + suspend/resume (ret + off)
>>> - omap4-panda-es: boot + suspend/resume
>>> - omap5-uevm: boot
>>> - dra7-evm: boot
>>> - am335x-bone: boot
>>>
>>> Test branches available:
>>>
>>> tree: https://github.com/t-kristo/linux-pm.git
>>
>>
>>> Fully functioning test branch: 3.12-rc6-dt-clks-v9
>>>
>> ^^ I tested this branch (boot testing):
>> Beagle-XM: http://pastebin.com/50A1qtFq (crashes + clkdm issues, dpll5
>> failed to transition)
>
> I just sent you a private email with a patch to try out, should fix the
> boot crash at least hopefully. Basically I forgot to convert one part of
> the kernel to the new regmap stuff for omap36xx.
it does bootup yes.
>
> clkdm issues are caused by wrong data in omap_hwmod_3xxx_data.c, USB
> nodes are listing l3_init_clkdm for them, but this only exists on
> omap4+. Seems like some copy paste bug introduced by someone.
>
> dpll5 part I am not too sure, can you check if the same happens with
> non-dt boot?
no-dt: http://pastebin.com/bYP9fTzH
dt: http://pastebin.com/xHup4L9Y
dpll5 warning seems to be only in dt-boot?
>>>
>>> Tracked this down: you were missing the following - looks like the
>>> conversion script might be missing converting the flags clock data
>>> over to dts?
>>>
>>> diff --git
>>> a/arch/arm/boot/dts/omap36xx-am35xx-omap3430es2plus-clocks.dtsi
>>> b/arch/arm/boot/dts/omap36xx-am35xx-omap3430es2plus-clocks.dtsi
>>> index 7e37e3e..c9b77c8 100644
>>> --- a/arch/arm/boot/dts/omap36xx-am35xx-omap3430es2plus-clocks.dtsi
>>> +++ b/arch/arm/boot/dts/omap36xx-am35xx-omap3430es2plus-clocks.dtsi
>>> @@ -30,6 +30,7 @@
>>> compatible = "ti,omap3-dpll-clock";
>>> clocks = <&sys_ck>, <&sys_ck>;
>>> reg = <0x0d04>, <0x0d24>, <0x0d34>, <0x0d4c>;
>>> + ti,low-power-stop;
>>> };
>>>
>>> dpll5_m2_ck: dpll5_m2_ck {
>>>
>>>
>>>
>>
>> Yea, seems I introduced the problem with the conversion script changes.
>> The valid fix for this is actually at the end of this mail (this fixes
>> both of the problems introduced, and also completes the fix you did), I
>> will add the fixes to the next rev.
>
> One debug feedback:
> reg = <0x0d04>, <0x0d24>, <0x0d34>, <0x0d4c>;
> clocks = <&sys_ck>, <&sys_ck>;
>
> we used indexing for varied register and clocks. however the register
> meaning per index changes based on type of DPLL. This was one painful
> thing to track down when debugging. the only robust option to debug
> was to use prints as part of register write/reads to ensure that right
> sequence was being followed.
>
> I understand based on review comments for dtb size, we have removed
> the names, but we lost sane debug capability as well with that.
>
I dont have any further feedback at this point beyond what is already
shared and so far my minimal tests have been good..
I have the following warnings:
sparse build warnings: http://pastebin.com/HZ1TWzyh
kernel-doc warnings: http://pastebin.com/JQwFEuaC
Hopefully, the next rev will not introducing nothing newer that what
we have in mainline.
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 32/43] ARM: dts: AM35xx: use DT clock data
On 10/25/2013 10:57 AM, Tero Kristo wrote:
[...]
> diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
> new file mode 100644
> index 000..c555443
> --- /dev/null
> +++ b/arch/arm/boot/dts/am3517.dtsi
> @@ -0,0 +1,31 @@
> +/*
> + * Device Tree Source for AM3517 SoC
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
> +
> +#include "omap3.dtsi"
> +
> +/ {
> + cpus {
> + cpu@0 {
> + /* OMAP343x/OMAP35xx variants OPP1-5 */
^^ you could fix the comment since this is OMAP35xx variant :)
> + operating-points = <
> + /* kHzuV */
> + 125000 975000
> + 25 1075000
> + 50 120
> + 55 127
> + 60 135
> + >;
> + clock-latency = <30>; /* From legacy driver */
> + };
> + };
> +};
[..]
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 33/43] ARM: dts: am43xx clock data
On 10/25/2013 10:57 AM, Tero Kristo wrote:
[...]
> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
> index 974d103..1fb3ac2 100644
> --- a/arch/arm/boot/dts/am4372.dtsi
> +++ b/arch/arm/boot/dts/am4372.dtsi
> @@ -67,6 +67,8 @@
> ranges;
> ti,hwmods = "l3_main";
>
> + /include/ "am43xx-clocks.dtsi"
> +
> edma: edma@4900 {
> compatible = "ti,edma3";
> ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
> diff --git a/arch/arm/boot/dts/am43xx-clocks.dtsi
> b/arch/arm/boot/dts/am43xx-clocks.dtsi
> new file mode 100644
> index 000..1cc5071
> --- /dev/null
> +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
> @@ -0,0 +1,666 @@
> +/*
> + * Device Tree Source for AM43xx clock data
> + *
> + * Copyright (C) 2013 Texas Instruments, Inc.
> + *
> + * 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.
> + */
> +scrm: scrm@44e1 {
> + compatible = "ti,scrm";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x44e1 0x2000>;
here and other dts -> could we keep the scrm, prm, cm devices in
SoC.dtsi? and clocks.dtsi just contains the clock nodes?
[...]
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 38/43] ARM: OMAP2+: PRM: add support for initializing PRCM clock modules from DT
On 10/25/2013 10:57 AM, Tero Kristo wrote:
[...]
> diff --git a/arch/arm/mach-omap2/prm_common.c
> b/arch/arm/mach-omap2/prm_common.c
> index 228b850..6fa74c6 100644
> --- a/arch/arm/mach-omap2/prm_common.c
> +++ b/arch/arm/mach-omap2/prm_common.c
[...]
> +/*
> + * XXX: implementation for the regmap read/write should be moved to
> + * individual PRCM IP drivers, once those are available.
> + */
> +static int ti_clk_regmap_read(void *context, unsigned int reg,
> + unsigned int *val)
> +{
> + void __iomem *mem = context;
> + *val = __raw_readl(mem + reg);
> + return 0;
> +}
> +
> +static int ti_clk_regmap_write(void *context, unsigned int reg,
> +unsigned int val)
> +{
> + void __iomem *mem = context;
> + __raw_writel(val, mem + reg);
> + return 0;
> +}
> +
> +static struct regmap_config ti_clk_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .reg_read = ti_clk_regmap_read,
> + .reg_write = ti_clk_regmap_write,
> + .fast_io = true,
> + .cache_type = REGCACHE_NONE,
> + .reg_format_endian = REGMAP_ENDIAN_NATIVE,
> + .val_format_endian = REGMAP_ENDIAN_NATIVE,
> +};
why not use regmap_mmio?
[...]
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 13/43] clk: ti: add support for basic mux clock
On 10/25/2013 10:57 AM, Tero Kristo wrote:
[...]
> diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
> new file mode 100644
> index 000..9c5259a
> --- /dev/null
> +++ b/drivers/clk/ti/mux.c
[...]
> +/**
> + * of_mux_clk_setup() - Setup function for simple mux rate clock
> + */
> +static int of_mux_clk_setup(struct device_node *node, struct regmap *regmap)
$ ./scripts/kernel-doc drivers/clk/ti/mux.c >/dev/null
Warning(drivers/clk/ti/mux.c:29): No description found for parameter
'node'
Warning(drivers/clk/ti/mux.c:29): No description found for parameter
'regmap'
I suggest in the next rev we do a verification if we have kernel doc
errors as well..
> +{
> + 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;
> + u32 flags = 0;
> + u32 val;
> +
> + 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 -EINVAL;
> + }
> + parent_names = kzalloc((sizeof(char *) * num_parents), GFP_KERNEL);
> + if (!parent_names) {
> + pr_err("%s: memory alloc failed\n", __func__);
as discussed, could be dropped.
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < num_parents; i++)
> + parent_names[i] = of_clk_get_parent_name(node, i);
> +
> + of_property_read_u32(node, "reg", &val);
is'nt this mandatory? error check?
> + reg = (void *)val;
> +
> + if (of_property_read_u32(node, "ti,bit-shift", &shift)) {
> + pr_debug("%s: bit-shift property defaults to 0x%x for %s\n",
> + __func__, shift, node->name);
why a debug if this is optional?
> + }
> +
> + if (of_property_read_bool(node, "ti,index-starts-at-one"))
> + clk_mux_flags |= CLK_MUX_INDEX_ONE;
> +
> + if (of_property_read_bool(node, "ti,set-rate-parent"))
> + flags |= CLK_SET_RATE_PARENT;
> +
> + /* Generate bit-mask based on parent info */
> + mask = num_parents;
> + if (!(clk_mux_flags & CLK_MUX_INDEX_ONE))
> + mask--;
we are assuming there wont be holes in the map (like reserved mux option?)
> +
> + mask = (1 << fls(mask)) - 1;
> +
> + clk = clk_register_mux_table_regmap(NULL, clk_name, parent_names,
> + num_parents, flags, reg, regmap,
> + shift, mask, clk_mux_flags, NULL,
> + NULL);
> +
> + if (!IS_ERR(clk)) {
> + of_clk_add_provider(node, of_clk_src_simple_get, clk);
> + return 0;
> + }
> +
kfree(parent_names)?
> + return PTR_ERR(clk);
> +}
> +CLK_OF_DECLARE(mux_clk, "ti,mux-clock", of_mux_clk_setup);
> +
> +static int __init of_ti_composite_mux_clk_setup(struct device_node *node,
> + struct regmap *regmap)
> +{
> + struct clk_mux *mux;
> + int num_parents;
> + int ret;
> + u32 val;
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return -ENOMEM;
> +
> + of_property_read_u32(node, "reg", &val);
is'nt this mandatory? error check?
> +
> + mux->reg = (void *)val;
> + mux->regmap = regmap;
> +
> + if (of_property_read_u32(node, "ti,bit-shift", &val)) {
> + pr_debug("%s: no bit-shift for %s, default=0\n",
> + __func__, node->name);
> + val = 0;
> + }
> + mux->shift = val;
> +
> + num_parents = of_clk_get_parent_count(node);
mandatory parameter without check?
ti,index-starts-at-one, ti,set-rate-parent
these seem not supported here even though the bindings dont tell us that.
> +
> + mux->mask = num_parents - 1;
> + mux->mask = (1 << fls(mux->mask)) - 1;
> +
> + ret = ti_clk_add_component(node, &mux->hw, CLK_COMPONENT_TYPE_MUX);
> + if (!ret)
> + return 0;
> +
> + kfree(mux);
> + return -ret;
> +}
> +CLK_OF_DECLARE(ti_composite_mux_clk_setup, "ti,composite-mux-clock",
> +of_ti_composite_mux_clk_setup);
>
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 12/43] CLK: TI: add support for clockdomain binding
On 10/25/2013 10:57 AM, Tero Kristo wrote:
[...]
> diff --git a/drivers/clk/ti/clockdomain.c b/drivers/clk/ti/clockdomain.c
> new file mode 100644
> index 000..1b3099e
> --- /dev/null
> +++ b/drivers/clk/ti/clockdomain.c
> @@ -0,0 +1,58 @@
> +/*
> + * OMAP clockdomain support
> + *
> + * 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
> +
> +static void __init of_ti_clockdomain_setup(struct device_node *node)
> +{
> + struct clk *clk;
> + struct clk_hw *clk_hw;
> + const char *clkdm_name = node->name;
> + int i;
> + int num_clks;
> +
> + num_clks = of_count_phandle_with_args(node, "clocks", "#clock-cells");
> +
> + for (i = 0; i < num_clks; i++) {
> + clk = of_clk_get(node, i);
> + if (__clk_get_flags(clk) & CLK_IS_BASIC) {
> + pr_warn("%s: can't setup clkdm for basic clk %s\n",
> + __func__, __clk_get_name(clk));
> + continue;
> + }
> + clk_hw = __clk_get_hw(clk);
> + to_clk_hw_omap(clk_hw)->clkdm_name = clkdm_name;
> + omap2_init_clk_clkdm(clk_hw);
eventually, this can disappear ofcourse.. and that will pave way to
remove the clock domain data to dts as well.. but yes, we have to find
a proper home for this away from current location..
> + }
> +}
> +
> +static struct of_device_id ti_clkdm_match_table[] __initdata = {
> + { .compatible = "ti,clockdomain" },
> + { }
> +};
> +
> +void __init ti_dt_clockdomains_setup(void)
since we are using of_clk_get, it will be good to provide
documentation w.r.t when the call is expected to be invoked.
> +{
> + struct device_node *np;
> + for_each_matching_node(np, ti_clkdm_match_table) {
> + of_ti_clockdomain_setup(np);
> + }
> +}
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v11 00/10] [PATCH v10 00/10] mtd:nand:omap2: clean-up of supported ECC schemes
Hi Tony, > From: Tony Lindgren > > * Brian Norris [131029 21:00]: > > Tony, you mentioned the DTS update in patch 8 going in via an ARM > > tree? This patch is not urgent, and it should probably wait until we > > know what release the rest of the series makes it into. This may > > depend on David Woodhouse's recommendation, but I'm not sure this > > series will have enough time baking in linux-next before entering > > mainline in 3.13 (the merge window is approaching). > > Yes Benoit or I can apply that patch if Pekon pings me or resends > that patch when it's OK to merge it. > Yes, I'll keep track of this and would resend you and Benoit the .dts patch separately when this these binding updates are merged in kernel. with regards, pekon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 11/43] CLK: TI: add support for gate clock
On 10/25/2013 10:57 AM, Tero Kristo wrote:
> This patch adds support for TI specific gate clocks. These behave as basic
> gate-clock, but have different ops / hw-ops for controlling the actual
> gate, for example waiting until the clock is ready. Several sub-types
> are supported:
> - ti,gate-clock: basic gate clock with default ops/hwops
> - ti,clkdm-gate-clock: clockdomain level gate control
> - ti,dss-gate-clock: gate clock with DSS specific hardware handling
> - ti,am35xx-gate-clock: gate clock with AM35xx specific hardware handling
> - ti,hsdiv-gate-clock: gate clock with OMAP36xx hardware errata handling
>
> Signed-off-by: Tero Kristo
> ---
> .../devicetree/bindings/clock/ti/gate.txt | 77 ++
> arch/arm/mach-omap2/clock.h| 29 ---
> drivers/clk/ti/Makefile|2 +-
> drivers/clk/ti/gate.c | 258
>
> include/linux/clk/ti.h | 36 +++
> 5 files changed, 372 insertions(+), 30 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt
> create mode 100644 drivers/clk/ti/gate.c
>
> diff --git a/Documentation/devicetree/bindings/clock/ti/gate.txt
> b/Documentation/devicetree/bindings/clock/ti/gate.txt
> new file mode 100644
> index 000..18c4d86
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti/gate.txt
> @@ -0,0 +1,77 @@
> +Binding for Texas Instruments gate clock.
> +
> +Binding status: Unstable - ABI compatibility may be broken in the future
> +
> +This binding uses the common clock binding[1]. This clock is
> +quite much similar to the basic gate-clock [2], however,
> +it supports a number of additional features. If no register
> +is provided for this clock, the code assumes that a clockdomain
> +will be controlled instead and the corresponding hw-ops for
> +that is used.
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +[2] Documentation/devicetree/bindings/clock/gate-clock.txt
> +[3] Documentation/devicetree/bindings/clock/ti/clockdomain.txt
i think you may want to sequence patch #12 before this if you would
like to refer to this.
> +
> +Required properties:
> +- compatible : shall be one of:
> + "ti,gate-clock" - basic gate clock
> + "ti,wait-gate-clock" - gate clock which waits until clock is active before
> + returning from clk_enable()
an example will be nice for this.
[...]
> diff --git a/drivers/clk/ti/gate.c b/drivers/clk/ti/gate.c
> new file mode 100644
> index 000..1a201f8
> --- /dev/null
> +++ b/drivers/clk/ti/gate.c
[...]
> +/**
> + * omap36xx_gate_clk_enable_with_hsdiv_restore - enable clocks suffering
> + * from HSDivider PWRDN problem Implements Errata ID: i556.
> + * @clk: DPLL output struct clk
> + *
> + * 3630 only: dpll3_m3_ck, dpll4_m2_ck, dpll4_m3_ck, dpll4_m4_ck,
> + * dpll4_m5_ck & dpll4_m6_ck dividers gets loaded with reset
> + * valueafter their respective PWRDN bits are set. Any dummy write
> + * (Any other value different from the Read value) to the
> + * corresponding CM_CLKSEL register will refresh the dividers.
> + */
> +static int omap36xx_gate_clk_enable_with_hsdiv_restore(struct clk_hw *clk)
> +{
> + struct clk_divider *parent;
> + struct clk_hw *parent_hw;
> + u32 dummy_v, orig_v;
> + int ret;
> +
> + /* Clear PWRDN bit of HSDIVIDER */
> + ret = omap2_dflt_clk_enable(clk);
> +
> + /* Parent is the x2 node, get parent of parent for the m2 div */
> + parent_hw = __clk_get_hw(__clk_get_parent(__clk_get_parent(clk->clk)));
> + parent = to_clk_divider(parent_hw);
> +
> + /* Restore the dividers */
> + if (!ret) {
> + orig_v = __raw_readl(parent->reg);
> + dummy_v = orig_v;
> +
> + /* Write any other value different from the Read value */
> + dummy_v ^= (1 << parent->shift);
> + __raw_writel(dummy_v, parent->reg);
> +
> + /* Write the original divider */
> + __raw_writel(orig_v, parent->reg);
i think we already did state that these need to be regmap_updatebits..
> + }
> +
> + return ret;
> +}
> +
> +static int __init _of_ti_gate_clk_setup(struct device_node *node,
> + const struct clk_ops *ops,
> + const struct clk_hw_omap_ops *hw_ops,
> + struct regmap *regmap)
> +{
> + struct clk *clk;
> + struct clk_init_data init = { NULL };
> + struct clk_hw_omap *clk_hw;
> + const char *clk_name = node->name;
> + const char *parent_name;
> + u32 val;
> +
> + clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
> + if (!clk_hw) {
> + pr_err("%s: could not allocate clk_hw_omap\n", __func__);
> + return -ENOMEM;
> + }
> +
> + clk_hw->hw.init = &init;
> +
> + of_property_read_string(node, "clock-output-names", &clk_name);
>
Re: [PATCHv9 09/43] CLK: ti: add support for ti divider-clock
On 11/01/2013 04:54 AM, Tero Kristo wrote:
> On 11/01/2013 11:48 AM, Tero Kristo wrote:
>> On 10/31/2013 08:02 PM, Nishanth Menon wrote:
>>> On 10/25/2013 10:57 AM, Tero Kristo wrote:
[...]
diff --git a/drivers/clk/ti/divider.c b/drivers/clk/ti/divider.c
new file mode 100644
index 000..787bc8f
--- /dev/null
+++ b/drivers/clk/ti/divider.c
[...]
+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.
>
> Some minor confusion here in my comment, sorry about that.
>
> Regmap is shared for the whole IP block and initialized only once, we
> can't remove it here as it is not owned by this clock. For
> clock-provider / unregister part, I don't want to clean those up as 1)
> unregister doesn't currently do anything 2) autoidle setup is not
> critical failure, it just breaks PM. I can add error print for it though.
Would IP blocks driven by clocks work with the autoidle configuration
broken? I believe we will be writing to DPLL_XYZ_GATE_CTRL? the usage
I believe is around
of_ti_clk_deny_autoidle_all/of_ti_clk_allow_autoidle_all which gets
invoked in omap2_clk_disable_autoidle_all part of xyz SoC init call.
is'nt the reason we do that to ensure we have no potential race when
clocks dissapear on their own without explicit control in critical
configuration paths?
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 08/43] clk: ti: add composite clock support
On 11/01/2013 04:35 AM, Tero Kristo wrote:
> On 10/31/2013 06:27 PM, Nishanth Menon wrote:
>> On 10/25/2013 10:57 AM, Tero Kristo wrote:
[..]
>>> diff --git a/drivers/clk/ti/composite.c b/drivers/clk/ti/composite.c
>>> new file mode 100644
>>> index 000..9ce7e54
>>> --- /dev/null
>>> +++ b/drivers/clk/ti/composite.c
[...]
>>
>>> + }
>>> +
>>> + 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?
>
> The list is shared between all composite clocks, as we don't have
> knowledge into which clock each component will go to at the time of the
> component registration. Consider this setup (comp = component):
>
> comp-a1
> comp-a2
> composite-a : comp-a1 + comp-a2
> comp-b1
> comp-b2
> comp-b3
> composite-b : comp-b1 + comp-b2 + comp-b3
>
> Depending on clock init order, we will potentially have 5 components in
> the list at max, which of 2 + 2 are of same type.
makes sense. Thanks for clarifying.
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 07/43] CLK: TI: add autoidle support
On 11/01/2013 04:18 AM, Tero Kristo wrote:
> On 10/31/2013 06:05 PM, Nishanth Menon wrote:
>> On 10/25/2013 10:57 AM, Tero Kristo wrote:
[...]
>>> diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
>>> new file mode 100644
>>> index 000..efa2a3e
>>> --- /dev/null
>>> +++ b/drivers/clk/ti/autoidle.c
[...]
>>> +}
>>> +
>>> +static void ti_deny_autoidle(struct clk_ti_autoidle *clk)
>>> +{
>>> + u32 val;
>>> +
>>> + regmap_read(clk->regmap, clk->reg, &val);
>>> +
>>> + if (clk->flags & AUTOIDLE_LOW)
>>> + val |= (1 << clk->shift);
>>> + else
>>> + val &= ~(1 << clk->shift);
>>> +
>>> + regmap_write(clk->regmap, clk->reg, val);
>> regmap_update_bits ?
>
> Same.
>
>>
>> and ofcourse error handling for regmap ops..
>
> What do you propose to do in error case? Panic, WARN() or just printk?
return error to caller and percolate back up the call stack.
one other thing I missed, will be nice to introduce a common bindings
for autoidle which tends to be reused in other drivers..
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 06/43] CLK: ti: add init support for clock IP blocks
On 11/01/2013 04:12 AM, Tero Kristo wrote:
> On 10/31/2013 05:42 PM, Nishanth Menon wrote:
>> On 10/25/2013 10:57 AM, Tero Kristo wrote:
>>> ti_dt_clk_init_provider() can now be used to initialize the contents of
>>> a single clock IP block. This parses all the clocks under the IP block
>>> and calls the corresponding init function for them.
>>>
>>> Signed-off-by: Tero Kristo
>>> ---
>>> drivers/clk/ti/clk.c | 59
>>>
>>> include/linux/clk/ti.h |1 +
>>> 2 files changed, 60 insertions(+)
>>>
>>> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
>>> index ad58b01..7f030d7 100644
>>> --- a/drivers/clk/ti/clk.c
>>> +++ b/drivers/clk/ti/clk.c
>>> @@ -19,6 +19,9 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> +
>>> +extern struct of_device_id __clk_of_table[];
>>>
>>> /**
>>>* ti_dt_clocks_register - register DT duplicate clocks during boot
>>> @@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk
>>> oclks[])
>>> }
>>> }
>>> }
>>> +
>>> +typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *);
>>> +
>>> +struct clk_init_item {
>>> + struct regmap *regmap;
>>> + struct device_node *np;
>>> + ti_of_clk_init_cb_t init_cb;
>>> + struct list_head node;
>>> +};
>>> +
>>> +static LIST_HEAD(retry_list);
>>> +
>>> +void __init ti_dt_clk_init_provider(struct device_node *parent,
>>> + struct regmap *regmap)
>>> +{
>>> + const struct of_device_id *match;
>>> + struct device_node *np;
>>> + ti_of_clk_init_cb_t clk_init_cb;
>>> + struct clk_init_item *retry;
>>> + struct clk_init_item *tmp;
>>> + int ret;
>>> +
>>> + for_each_child_of_node(parent, np) {
>>> + match = of_match_node(__clk_of_table, np);
>>> + if (!match)
>>> + continue;
>>> + clk_init_cb = match->data;
>>
>> I must admit I am confused here.
>
> Yea this patch is something I am not quite comfortable myself yet and
> would like improvement ideas
>
>> a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match
>> data. The prototype of the generic of_clk_init_cb_t is typedef void
>> (*of_clk_init_cb_t)(struct device_node *);
>> b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes
>> from __clk_of_table
>
> __clk_of_table contains the function pointers for the clock init
> functions, not clock nodes.
yes, apologies, should have stated the prototypes of functions in a
single __clk_of_table should not be two different argument list. The
reason is as follows: CLK_OF_DECLARE fills up that list. there can be
generic drivers such as [1] which might be registered, OR in the case
of MULTI_ARCH - multiple other SoC drivers would have registered
there. There is a bunch of stuff we could mess up here.
If we do not have a choice, if we would like to maintain a different
prototype based init list, we should probably do it in a different
structure - example __clk_of_table_regmap or something similar..
>
>>
>> I assume we need ti_dt_clk_init_provider to be always called with
>> clock list, as a result of_clk_init(NULL); will never be invoked. This
>> is counter productive as you have have generic non SoC clock providers
>> as well who would have been invoked with of_clk_init(NULL);
>
> Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock
> nodes under respective IP blocks. Two reasons here:
>
> a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to
> the clock init functions, basically the pointer to the memory map region
> (regmap.)
> b) of_clk_init(NULL) will initialize all the clock nodes in the system,
> irrespective of the hierarchy considerations.
>
> Only thing that can be done, is to make the API introduced in this patch
> a generic API and call it something like of_clk_init_children().
you still can do that:
of_prcm_init->ti_dt_clk_init_provider-> instead of matching nodes from
__clk_of_table, you can make it __clk_of_regmap_table -> the
CLK_OF_DECLARE will not be good enough here ofcourse, but this will
probably belong to a generic regmap enabled clock support framework -
i dont see much that is TI specific in drivers/clk/ti/clk.c and it
makes sense to be part of generic clock framework handling as generic
clock framework will receive regmap support based on previous patches.
that allows for other platforms to use regmap based clk drivers as well.
[1] http://marc.info/?l=linux-omap&m=138331451210184&w=2
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC] clk: add gpio controlled clock
The added clk-gpio is a basic clock that can be enabled and disabled
trough a gpio output. The DT binding document for the clock is also
added.
Signed-off-by: Jyri Sarha
---
.../devicetree/bindings/clock/gpio-clock.txt | 21 +++
drivers/clk/Makefile |1 +
drivers/clk/clk-gpio.c | 154
include/linux/clk-provider.h | 25
4 files changed, 201 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/gpio-clock.txt
create mode 100644 drivers/clk/clk-gpio.c
diff --git a/Documentation/devicetree/bindings/clock/gpio-clock.txt
b/Documentation/devicetree/bindings/clock/gpio-clock.txt
new file mode 100644
index 000..54fea39
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/gpio-clock.txt
@@ -0,0 +1,21 @@
+Binding for simple gpio controlled clock.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "gpio-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- enable-gpios : GPIO reference for enabling and disabling the clock.
+
+Optional properties:
+- clocks: Maximum of one parent clock is supported.
+
+Example:
+ clock {
+ compatible = "gpio-clock";
+ clocks = <&parentclk>;
+ #clock-cells = <0>;
+ enable-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
+ };
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 7d74d06..81b65a3 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)+= clk-fixed-rate.o
obj-$(CONFIG_COMMON_CLK) += clk-gate.o
obj-$(CONFIG_COMMON_CLK) += clk-mux.o
obj-$(CONFIG_COMMON_CLK) += clk-composite.o
+obj-$(CONFIG_COMMON_CLK) += clk-gpio.o
# SoCs specific
obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o
diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
new file mode 100644
index 000..54f21d9
--- /dev/null
+++ b/drivers/clk/clk-gpio.c
@@ -0,0 +1,154 @@
+/*
+ * Copyright (C) 2012 Texas Instruments
+ * Author: Jyri Sarha
+ *
+ * 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.
+ *
+ * Gpio controlled clock implementation
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * DOC: basic gpio controlled clock which can be enabled and disabled
+ * with gpio output
+ * Traits of this clock:
+ * prepare - clk_(un)prepare only ensures parent is (un)prepared
+ * enable - clk_enable and clk_disable are functional & control gpio
+ * rate - inherits rate from parent. No clk_set_rate support
+ * parent - fixed parent. No clk_set_parent support
+ */
+
+#define to_clk_gpio(_hw) container_of(_hw, struct clk_gpio, hw)
+
+static int clk_gpio_enable(struct clk_hw *hw)
+{
+ struct clk_gpio *gpio = to_clk_gpio(hw);
+ int value = gpio->active_low ? 0 : 1;
+
+ gpio_set_value(gpio->gpio, value);
+
+ return 0;
+}
+
+static void clk_gpio_disable(struct clk_hw *hw)
+{
+ struct clk_gpio *gpio = to_clk_gpio(hw);
+ int value = gpio->active_low ? 1 : 0;
+
+ gpio_set_value(gpio->gpio, value);
+}
+
+static int clk_gpio_is_enabled(struct clk_hw *hw)
+{
+ struct clk_gpio *gpio = to_clk_gpio(hw);
+ int value = gpio_get_value(gpio->gpio);
+
+ return gpio->active_low ? !value : value;
+}
+
+const struct clk_ops clk_gpio_ops = {
+ .enable = clk_gpio_enable,
+ .disable = clk_gpio_disable,
+ .is_enabled = clk_gpio_is_enabled,
+};
+EXPORT_SYMBOL_GPL(clk_gpio_ops);
+
+/**
+ * clk_register_gpio - register a gpip clock with the clock framework
+ * @dev: device that is registering this clock
+ * @name: name of this clock
+ * @parent_name: name of this clock's parent
+ * @flags: framework-specific flags for this clock
+ * @gpio: gpio to control this clock
+ * @active_low: gpio polarity
+ */
+struct clk *clk_register_gpio(struct device *dev, const char *name,
+ const char *parent_name, unsigned long flags,
+ unsigned int gpio, bool active_low)
+{
+ struct clk_gpio *clk_gpio;
+ struct clk *clk;
+ struct clk_init_data init = { NULL };
+ unsigned long gpio_flags;
+ int err;
+
+ if (active_low)
+ gpio_flags = GPIOF_OUT_INIT_LOW;
+ else
+ gpio_flags = GPIOF_OUT_INIT_HIGH;
+
+ err = gpio_request_one(gpio, gpio_flags, name);
+ if (err) {
+ pr_err("%s: Error requesting clock control gpio %u\n",
+ __func__, gpio);
+ return ERR_PTR(-EINVAL);
+ }
+
+ clk_gpio = kzalloc(sizeof(struct clk_gpio), GFP_KERNEL);
+ if (!clk_gpio) {
+ pr_err("%s: could not allocate gpio clk\n", __func_
[PATCH RFC] gpio controlled clock
The patch implements a basic clock that can be enabled and disabled trough a gpio output. There is such a clock at least on Beaglebone-Black and I need such a clock to implement HDMI audio support for the board. I just thought this simple driver could be useful for wider audience. Best regards, Jyri Jyri Sarha (1): clk: add gpio controlled clock .../devicetree/bindings/clock/gpio-clock.txt | 21 +++ drivers/clk/Makefile |1 + drivers/clk/clk-gpio.c | 154 include/linux/clk-provider.h | 25 4 files changed, 201 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/gpio-clock.txt create mode 100644 drivers/clk/clk-gpio.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 09/43] CLK: ti: add support for ti divider-clock
On 11/01/2013 11:48 AM, Tero Kristo wrote:
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 000..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 valueactual divisor value
+01
+12
+23
+
+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 valueactual divisor value
+11
+22
+33
+
+ti,index-power-of-two - valid divisor values are powers of two. E.g:
+register valueactual divisor value
+01
+12
+24
+
+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 valueactual divisor value
+04
+18
+2
+316
+
+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/cl
Re: [PATCHv9 10/43] clk: ti: add support for TI fixed factor clock
On 10/31/2013 08:12 PM, Nishanth Menon wrote:
On 10/25/2013 10:57 AM, Tero Kristo wrote:
This behaves exactly in similar manner to basic fixed-factor-clock, but
adds a few properties on top for handling clock hardware autoidling.
Signed-off-by: Tero Kristo
---
.../bindings/clock/ti/fixed-factor-clock.txt | 29 +
drivers/clk/ti/Makefile|2 +-
drivers/clk/ti/fixed-factor.c | 65
3 files changed, 95 insertions(+), 1 deletion(-)
create mode 100644
Documentation/devicetree/bindings/clock/ti/fixed-factor-clock.txt
create mode 100644 drivers/clk/ti/fixed-factor.c
diff --git a/Documentation/devicetree/bindings/clock/ti/fixed-factor-clock.txt
b/Documentation/devicetree/bindings/clock/ti/fixed-factor-clock.txt
new file mode 100644
index 000..60b9e34
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti/fixed-factor-clock.txt
@@ -0,0 +1,29 @@
+Binding for TI fixed factor rate clock sources.
+
+Binding status: Unstable - ABI compatibility may be broken in the future
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "ti,fixed-factor-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- ti,clock-div: fixed divider.
+- ti,clock-mult: fixed multiplier.
+- clocks: parent clock.
+
+Optional properties:
+- ti,autoidle-shift: bit shift of the autoidle enable bit for the clock
+- reg: offset for the autoidle register of this clock
+- ti,invert-autoidle-bit: autoidle is enabled by setting the bit to 0
The above three belong to autoidle stuff I think.. maybe pointing to
it's binding will help?
+- ti,set-rate-parent: clk_set_rate is propagated to parent
+
+Example:
+ clock {
+ compatible = "ti,fixed-factor-clock";
+ clocks = <&parentclk>;
+ #clock-cells = <0>;
+ ti,clock-div = <2>;
+ ti,clock-mult = <1>;
+ };
diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
index 640ebf9..f57fc4b 100644
--- a/drivers/clk/ti/Makefile
+++ b/drivers/clk/ti/Makefile
@@ -1,4 +1,4 @@
ifneq ($(CONFIG_OF),)
obj-y += clk.o dpll.o autoidle.o divider.o \
- composite.o
+ fixed-factor.o composite.o
endif
diff --git a/drivers/clk/ti/fixed-factor.c b/drivers/clk/ti/fixed-factor.c
new file mode 100644
index 000..e0549c6
--- /dev/null
+++ b/drivers/clk/ti/fixed-factor.c
@@ -0,0 +1,65 @@
+/*
+ * TI Fixed Factor 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
+
+/**
+ * of_ti_fixed_factor_clk_setup() - Setup function for TI fixed factor clock
+ */
+static int __init of_ti_fixed_factor_clk_setup(struct device_node *node,
+ struct regmap *regmap)
+{
+ struct clk *clk;
+ const char *clk_name = node->name;
+ const char *parent_name;
+ u32 div, mult;
+ u32 flags = 0;
+
+ if (of_property_read_u32(node, "ti,clock-div", &div)) {
+ pr_err("%s Fixed factor clock <%s> must have a clock-div
property\n",
+ __func__, node->name);
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32(node, "ti,clock-mult", &mult)) {
+ pr_err("%s Fixed factor clock <%s> must have a clokc-mult
property\n",
+ __func__, node->name);
+ return -EINVAL;
+ }
+
+ if (of_property_read_bool(node, "ti,set-rate-parent"))
+ flags |= CLK_SET_RATE_PARENT;
+
+ parent_name = of_clk_get_parent_name(node, 0);
+
+ clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
+ mult, div);
+
+ if (!IS_ERR(clk)) {
+ of_clk_add_provider(node, of_clk_src_simple_get, clk);
+ return of_ti_autoidle_setup(node, regmap);
if this fails, remove provider and unregister?
Unregister is not supported currently. I don't think I want to do
partial cleanup here, failing autoidle setup is not critical anyway. The
system will still boot-up properly, only PM is potentially broken. Can
add an error print though.
-Tero
+ }
+
+ return PTR_ERR(clk);
+}
+CLK_OF_DECLARE(ti_fixed_factor_clk, "ti,fix
Re: [PATCHv9 09/43] CLK: ti: add support for ti divider-clock
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 000..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
Re: [PATCHv9 08/43] clk: ti: add composite clock support
On 10/31/2013 06:32 PM, Nishanth Menon wrote:
On 10/31/2013 11:27 AM, Nishanth Menon wrote:
On 10/25/2013 10:57 AM, Tero Kristo wrote:
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
---
.../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.c
diff --git a/Documentation/devicetree/bindings/clock/ti/composite.txt
b/Documentation/devicetree/bindings/clock/ti/composite.txt
new file mode 100644
index 000..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>;
+};
From Topology perspective, would this be a better approach?
usb_l4_ick: usb_l4_ick {
#clock-cells = <0>;
compatible = "ti,composite-clock";
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;
};
};
Well I was considering this, however this would require extra level of
registration also:
of_ti_composite_clk_setup ->
(match children against composite types)
of_ti_composite_divider_setup
of_ti_composite_gate_setup
Might be cleaner overall though, so I can take a look at this.
-Tero
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 08/43] clk: ti: add composite clock support
On 10/31/2013 06:27 PM, Nishanth Menon wrote:
On 10/25/2013 10:57 AM, Tero Kristo wrote:
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
---
.../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.c
diff --git a/Documentation/devicetree/bindings/clock/ti/composite.txt
b/Documentation/devicetree/bindings/clock/ti/composite.txt
new file mode 100644
index 000..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
endif
diff --git a/drivers/clk/ti/composite.c b/drivers/clk/ti/composite.c
new file mode 100644
index 000..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
+ *
+ * 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
+#include
+
+#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
Re: [PATCHv9 07/43] CLK: TI: add autoidle support
On 10/31/2013 06:05 PM, Nishanth Menon wrote:
On 10/25/2013 10:57 AM, Tero Kristo wrote:
TI clk driver now routes some of the basic clocks through own
registration routine to allow autoidle support. This routine just
checks a couple of device node properties and adds autoidle support
if required, and just passes the registration forward to basic clocks.
Signed-off-by: Tero Kristo
---
arch/arm/mach-omap2/clock.c |6 +++
drivers/clk/ti/Makefile |2 +-
drivers/clk/ti/autoidle.c | 109 +++
include/linux/clk/ti.h |9
4 files changed, 125 insertions(+), 1 deletion(-)
create mode 100644 drivers/clk/ti/autoidle.c
diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
index 0c38ca9..223f432b 100644
--- a/arch/arm/mach-omap2/clock.c
+++ b/arch/arm/mach-omap2/clock.c
@@ -520,6 +520,9 @@ int omap2_clk_enable_autoidle_all(void)
list_for_each_entry(c, &clk_hw_omap_clocks, node)
if (c->ops && c->ops->allow_idle)
c->ops->allow_idle(c);
+
+ of_ti_clk_allow_autoidle_all();
+
return 0;
}
@@ -539,6 +542,9 @@ int omap2_clk_disable_autoidle_all(void)
list_for_each_entry(c, &clk_hw_omap_clocks, node)
if (c->ops && c->ops->deny_idle)
c->ops->deny_idle(c);
+
+ of_ti_clk_deny_autoidle_all();
+
return 0;
}
diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
index 05af5d8..533efb4 100644
--- a/drivers/clk/ti/Makefile
+++ b/drivers/clk/ti/Makefile
@@ -1,3 +1,3 @@
ifneq ($(CONFIG_OF),)
-obj-y += clk.o dpll.o
+obj-y += clk.o dpll.o autoidle.o
endif
diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
new file mode 100644
index 000..efa2a3e
--- /dev/null
+++ b/drivers/clk/ti/autoidle.c
@@ -0,0 +1,109 @@
+/*
+ * TI clock autoidle support
+ *
+ * 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
+
+struct clk_ti_autoidle {
+ u32 reg;
+ struct regmap *regmap;
+ u8 shift;
+ u8 flags;
+ const char *name;
+ struct list_headnode;
+};
+
+#define AUTOIDLE_LOW 0x1
+
+static LIST_HEAD(autoidle_clks);
+
+static void ti_allow_autoidle(struct clk_ti_autoidle *clk)
+{
+ u32 val;
+
+ regmap_read(clk->regmap, clk->reg, &val);
+
+ if (clk->flags & AUTOIDLE_LOW)
+ val &= ~(1 << clk->shift);
+ else
+ val |= (1 << clk->shift);
+
+ regmap_write(clk->regmap, clk->reg, val);
regmap_update_bits ?
Hmm yea that would work.
+}
+
+static void ti_deny_autoidle(struct clk_ti_autoidle *clk)
+{
+ u32 val;
+
+ regmap_read(clk->regmap, clk->reg, &val);
+
+ if (clk->flags & AUTOIDLE_LOW)
+ val |= (1 << clk->shift);
+ else
+ val &= ~(1 << clk->shift);
+
+ regmap_write(clk->regmap, clk->reg, val);
regmap_update_bits ?
Same.
and ofcourse error handling for regmap ops..
What do you propose to do in error case? Panic, WARN() or just printk?
+}
+
+void of_ti_clk_allow_autoidle_all(void)
+{
+ struct clk_ti_autoidle *c;
+
+ list_for_each_entry(c, &autoidle_clks, node)
+ ti_allow_autoidle(c);
+}
+
+void of_ti_clk_deny_autoidle_all(void)
+{
+ struct clk_ti_autoidle *c;
+
+ list_for_each_entry(c, &autoidle_clks, node)
+ ti_deny_autoidle(c);
+}
+
+int __init of_ti_autoidle_setup(struct device_node *node,
+ struct regmap *regmap)
will be nice if you could kernel-doc all public functions at the very
least.
Ok, I can try to do that for next. :P
+{
+ u32 shift;
+ struct clk_ti_autoidle *clk;
+
+ if (of_property_read_u32(node, "ti,autoidle-shift", &shift))
+ return 0;
+
+ clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+
+ if (!clk) {
+ pr_err("%s: kzalloc failed\n", __func__);
+ return -ENOMEM;
+ }
+
+ clk->shift = shift;
+ clk->name = node->name;
+ of_property_read_u32(node, "reg", &clk->reg);
dt binding?
what happens if someone forgets this?
You get zero offset.
I can add a check for this though.
+ clk->regmap = regmap;
+
+ if (of_property_read_bool(node, "ti,invert-aut
Re: [PATCHv9 06/43] CLK: ti: add init support for clock IP blocks
On 10/31/2013 05:42 PM, Nishanth Menon wrote:
On 10/25/2013 10:57 AM, Tero Kristo wrote:
ti_dt_clk_init_provider() can now be used to initialize the contents of
a single clock IP block. This parses all the clocks under the IP block
and calls the corresponding init function for them.
Signed-off-by: Tero Kristo
---
drivers/clk/ti/clk.c | 59
include/linux/clk/ti.h |1 +
2 files changed, 60 insertions(+)
diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
index ad58b01..7f030d7 100644
--- a/drivers/clk/ti/clk.c
+++ b/drivers/clk/ti/clk.c
@@ -19,6 +19,9 @@
#include
#include
#include
+#include
+
+extern struct of_device_id __clk_of_table[];
/**
* ti_dt_clocks_register - register DT duplicate clocks during boot
@@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
}
}
}
+
+typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *);
+
+struct clk_init_item {
+ struct regmap *regmap;
+ struct device_node *np;
+ ti_of_clk_init_cb_t init_cb;
+ struct list_head node;
+};
+
+static LIST_HEAD(retry_list);
+
+void __init ti_dt_clk_init_provider(struct device_node *parent,
+ struct regmap *regmap)
+{
+ const struct of_device_id *match;
+ struct device_node *np;
+ ti_of_clk_init_cb_t clk_init_cb;
+ struct clk_init_item *retry;
+ struct clk_init_item *tmp;
+ int ret;
+
+ for_each_child_of_node(parent, np) {
+ match = of_match_node(__clk_of_table, np);
+ if (!match)
+ continue;
+ clk_init_cb = match->data;
I must admit I am confused here.
Yea this patch is something I am not quite comfortable myself yet and
would like improvement ideas
a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match
data. The prototype of the generic of_clk_init_cb_t is typedef void
(*of_clk_init_cb_t)(struct device_node *);
b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes
from __clk_of_table
__clk_of_table contains the function pointers for the clock init
functions, not clock nodes.
I assume we need ti_dt_clk_init_provider to be always called with
clock list, as a result of_clk_init(NULL); will never be invoked. This
is counter productive as you have have generic non SoC clock providers
as well who would have been invoked with of_clk_init(NULL);
Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock
nodes under respective IP blocks. Two reasons here:
a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to
the clock init functions, basically the pointer to the memory map region
(regmap.)
b) of_clk_init(NULL) will initialize all the clock nodes in the system,
irrespective of the hierarchy considerations.
Only thing that can be done, is to make the API introduced in this patch
a generic API and call it something like of_clk_init_children().
I do strongly encourage using of_clk_init(NULL) and not having to
switch the clk_init call back with SoC specific one as it forces
un-intended dependency.
Can't do this as mentioned above.
Further such as SoC specific callback should have been in a header.
+ pr_debug("%s: initializing: %s\n", __func__, np->name);
one further comment - using pr_fmt will save us from using __func__
for every pr_* message :(
Hmm good comment, will fix that for next rev.
+ ret = clk_init_cb(np, regmap);
+ if (ret == -EAGAIN) {
+ pr_debug("%s: adding to again list...\n", np->name);
+ retry = kzalloc(sizeof(*retry), GFP_KERNEL);
+ retry->np = np;
+ retry->regmap = regmap;
+ retry->init_cb = clk_init_cb;
+ list_add(&retry->node, &retry_list);
+ } else if (ret) {
+ pr_err("%s: clock init failed for %s (%d)!\n", __func__,
+ np->name, ret);
+ }
+ }
+
+ list_for_each_entry_safe(retry, tmp, &retry_list, node) {
+ pr_debug("%s: retry-init: %s\n", __func__, retry->np->name);
+ ret = retry->init_cb(retry->np, retry->regmap);
+ if (ret == -EAGAIN) {
+ pr_debug("%s failed again?\n", retry->np->name);
+ } else {
+ if (ret)
+ pr_err("%s: clock init failed for %s (%d)!\n",
+ __func__, retry->np->name, ret);
+ list_del(&retry->node);
+ kfree(retry);
+ }
+ }
+}
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index 9786752..7ab02fa 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -177,6 +177,7 @@ int omap3_dpll4_
Re: [PATCHv9 01/43] clk: Add support for regmap register read/write
On 10/31/2013 05:46 PM, Nishanth Menon wrote:
On 10/31/2013 09:40 AM, Tero Kristo wrote:
On 10/31/2013 04:03 PM, Nishanth Menon wrote:
On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 7e59253..63ff78c 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
[...]
-static inline u32 clk_readl(u32 __iomem *reg)
+static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap)
{
- return readl(reg);
+ u32 val;
+
+ if (regmap)
+ regmap_read(regmap, (u32)reg, &val);
+ else
+ val = readl(reg);
+ return val;
}
-static inline void clk_writel(u32 val, u32 __iomem *reg)
+static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap)
{
- writel(val, reg);
+ if (regmap)
+ regmap_write(regmap, (u32)reg, val);
+ else
+ writel(val, reg);
}
#endif /* CONFIG_COMMON_CLK */
Might it not be better to introduce regmap variants?
static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap
*regmap)
and corresponding readl? that allows cleaner readability for clk
drivers that use regmap and those that dont.
Well, doing that will introduce a lot of redundant code, as the checks
for the presence of regmap must be copied all over the place. With this
patch, all the generic clock drivers support internally both regmap or
non-regmap register accesses.
using function pointers might be an appropriate solution. in general a
low level reg access api that uses two different approaches sounds a
little.. umm.. fishy..
Initial work I did for v9 had clk_reg_ops struct in place which pretty
much did this, however Mike recommended looking at the regmap so I did.
I could put the reg_ops struct back and just have it use internally
regmap if that would be better, but I guess we need comment from Mike
how he wants this to be done.
We could have:
struct clk_reg_ops {
int (*clk_readl)(u32 addr, u32 *val);
int (*clk_writel)(u32 addr, u32 val);
};
struct clk_foo {
...
void __iomem *reg;
struct clk_reg_ops *regops;
};
regmap can also return error value that could also be handled as a result.
True, however if the regmap fails for the clock code, not sure what we
can do but panic. If this code is expanded, it is probably better to not
inline it anymore.
let the compiler deal with that decision. regmap operation fail should
be percollated back to initiator of the request. in some cases that
will be ir-recoverable, but on other cases panic might be the right
answer - at the low level we are in no position to make that distinction.
Currently, none of the clock drivers handle failures for regmap
operations, I can of course add some sort of support for this given what
we do with above point and what the API for the register accesses ends
up like.
-Tero
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
