From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tero Kristo Subject: Re: [PATCHv4 06/33] CLK: omap: add autoidle support Date: Wed, 31 Jul 2013 13:13:57 +0300 Message-ID: <51F8E365.8080604@ti.com> References: <1374564028-11352-1-git-send-email-t-kristo@ti.com> <1374564028-11352-7-git-send-email-t-kristo@ti.com> <51F80C77.9020109@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51F80C77.9020109@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Nishanth Menon Cc: paul@pwsan.com, khilman@linaro.org, mturquette@linaro.org, tony@atomide.com, devicetree-discuss@lists.ozlabs.org, rnayak@ti.com, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org On 07/30/2013 09:56 PM, Nishanth Menon wrote: > On 07/23/2013 02:20 AM, Tero Kristo wrote: >> OMAP 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. > > why not extend standard framework to support autoidle capable clocks OR > introduce our own clk node which depends on basic clocks? Was kind of easier this way. >> >> Signed-off-by: Tero Kristo >> --- >> arch/arm/mach-omap2/clock.c | 6 ++ >> drivers/clk/omap/Makefile | 2 +- >> drivers/clk/omap/autoidle.c | 130 >> +++++++++++++++++++++++++++++++++++++++++++ >> drivers/clk/omap/clk.c | 4 +- >> include/linux/clk/omap.h | 4 ++ >> 5 files changed, 143 insertions(+), 3 deletions(-) >> create mode 100644 drivers/clk/omap/autoidle.c > > I know it is getting a little stale, but anyways, device tree binding > missing. > >> >> diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c >> index 0c38ca9..669d4c4 100644 >> --- a/arch/arm/mach-omap2/clock.c >> +++ b/arch/arm/mach-omap2/clock.c > Not sure why, at this point in time we are going to calling drivers/clk > code. > >> @@ -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_omap_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_omap_clk_deny_autoidle_all(); >> + > > these are defined for CONFIG_OF.. anyways, without dt nodes (OMAP3 is > supposed to support non-DT boot even now), this would not work, would it? The lists are empty so the funcs do nothing. However, dropping CONFIG_OF would break these of course. Will figure out a fix for this. The calls are needed for the transition phase until we can move more clk stuff from mach-omap2 to drivers. > > >> return 0; >> } >> >> diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile >> index 4cad480..ca56700 100644 >> --- a/drivers/clk/omap/Makefile >> +++ b/drivers/clk/omap/Makefile >> @@ -1 +1 @@ >> -obj-y += clk.o dpll.o >> +obj-y += clk.o dpll.o autoidle.o >> diff --git a/drivers/clk/omap/autoidle.c b/drivers/clk/omap/autoidle.c >> new file mode 100644 >> index 0000000..6424cb2 >> --- /dev/null >> +++ b/drivers/clk/omap/autoidle.c >> @@ -0,0 +1,130 @@ >> +/* >> + * OMAP 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 >> +#include >> +#include >> +#include > at all of these required? I'll double check. > >> + >> +#ifdef CONFIG_OF >> +struct clk_omap_autoidle { >> + void __iomem *reg; >> + u8 shift; >> + u8 flags; >> + const char *name; >> + struct list_head node; >> +}; >> + >> +#define AUTOIDLE_LOW 0x1 >> + >> +static LIST_HEAD(autoidle_clks); >> + >> +static void omap_allow_autoidle(struct clk_omap_autoidle *clk) >> +{ >> + u32 val; >> + >> + val = readl(clk->reg); >> + >> + if (clk->flags & AUTOIDLE_LOW) >> + val &= ~(1 << clk->shift); >> + else >> + val |= (1 << clk->shift); >> + >> + writel(val, clk->reg); >> +} >> + >> +static void omap_deny_autoidle(struct clk_omap_autoidle *clk) >> +{ >> + u32 val; >> + >> + val = readl(clk->reg); >> + >> + if (clk->flags & AUTOIDLE_LOW) >> + val |= (1 << clk->shift); >> + else >> + val &= ~(1 << clk->shift); >> + >> + writel(val, clk->reg); >> +} >> + >> +void of_omap_clk_allow_autoidle_all(void) >> +{ >> + struct clk_omap_autoidle *c; >> + >> + list_for_each_entry(c, &autoidle_clks, node) >> + omap_allow_autoidle(c); >> +} >> + >> +void of_omap_clk_deny_autoidle_all(void) >> +{ >> + struct clk_omap_autoidle *c; >> + >> + list_for_each_entry(c, &autoidle_clks, node) >> + omap_deny_autoidle(c); >> +} >> + >> +static __init void of_omap_autoidle_setup(struct device_node *node) >> +{ >> + u32 shift; >> + void __iomem *reg; >> + struct clk_omap_autoidle *clk; >> + >> + if (of_property_read_u32(node, "ti,autoidle-shift", &shift)) >> + return; >> + >> + reg = of_iomap(node, 0); >> + >> + clk = kzalloc(sizeof(struct clk_omap_autoidle), GFP_KERNEL); >> + >> + if (!clk) { >> + pr_err("%s: kzalloc failed\n", __func__); >> + return; >> + } >> + >> + clk->shift = shift; >> + clk->name = node->name; >> + clk->reg = reg; >> + >> + if (of_property_read_bool(node, "ti,autoidle-low")) >> + clk->flags |= AUTOIDLE_LOW; >> + >> + list_add(&clk->node, &autoidle_clks); >> +} >> + >> +void __init of_omap_divider_setup(struct device_node *node) >> +{ >> + of_divider_clk_setup(node); >> + of_omap_autoidle_setup(node); >> +} >> +EXPORT_SYMBOL_GPL(of_omap_divider_setup); >> +CLK_OF_DECLARE(omap_autoidle_clock, "divider-clock", >> of_omap_divider_setup); > > This is overriding drivers/clk/clk-divider.c ? Yea, this declaration here should be removed. >> + >> +void __init of_omap_fixed_factor_setup(struct device_node *node) >> +{ >> + of_fixed_factor_clk_setup(node); >> + of_omap_autoidle_setup(node); >> +} >> +EXPORT_SYMBOL_GPL(of_omap_fixed_factor_setup); >> +CLK_OF_DECLARE(omap_fixed_factor_clock, "fixed-factor-clock", >> + of_omap_fixed_factor_setup); > > This is overriding drivers/clk/clk-fixed-factor.c ? Ditto. >> + >> +#endif >> diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c >> index cd31a81..c149097 100644 >> --- a/drivers/clk/omap/clk.c >> +++ b/drivers/clk/omap/clk.c >> @@ -25,8 +25,8 @@ static const struct of_device_id clk_match[] = { >> {.compatible = "fixed-clock", .data = of_fixed_clk_setup, }, >> {.compatible = "mux-clock", .data = of_mux_clk_setup, }, >> {.compatible = "fixed-factor-clock", >> - .data = of_fixed_factor_clk_setup, }, >> - {.compatible = "divider-clock", .data = of_divider_clk_setup, }, >> + .data = of_omap_fixed_factor_setup, }, >> + {.compatible = "divider-clock", .data = of_omap_divider_setup, }, These should be enough for registration. >> {.compatible = "gate-clock", .data = of_gate_clk_setup, }, >> {.compatible = "ti,omap4-dpll-clock", .data = >> of_omap4_dpll_setup, }, >> {}, >> diff --git a/include/linux/clk/omap.h b/include/linux/clk/omap.h >> index c39e775..904bdad 100644 >> --- a/include/linux/clk/omap.h >> +++ b/include/linux/clk/omap.h >> @@ -192,5 +192,9 @@ extern const struct clk_hw_omap_ops >> clkhwops_omap4_dpllmx; >> int dt_omap_clk_init(void); >> extern void omap_dt_clocks_register(struct omap_dt_clk *oclks, int >> cnt); >> void of_omap4_dpll_setup(struct device_node *node); >> +void of_omap_fixed_factor_setup(struct device_node *node); >> +void of_omap_divider_setup(struct device_node *node); >> +void of_omap_clk_allow_autoidle_all(void); >> +void of_omap_clk_deny_autoidle_all(void); >> >> #endif >> > > I personally dont prefer the fact that divider-clock and > fixed-rate-clock now has double meaning - building a multi-arch kernel > for example, this can wreak havoc. standard definitions should not be > monkeyed around with thus if avoidable, and in this case, very much > avoidable. > > just my 2 cents. Yea, as described before, I'll change the code not to make a global override, instead, just make omap specific override which parses the extra params. That sound better or still see some issues with that? -Tero From mboxrd@z Thu Jan 1 00:00:00 1970 From: t-kristo@ti.com (Tero Kristo) Date: Wed, 31 Jul 2013 13:13:57 +0300 Subject: [PATCHv4 06/33] CLK: omap: add autoidle support In-Reply-To: <51F80C77.9020109@ti.com> References: <1374564028-11352-1-git-send-email-t-kristo@ti.com> <1374564028-11352-7-git-send-email-t-kristo@ti.com> <51F80C77.9020109@ti.com> Message-ID: <51F8E365.8080604@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 07/30/2013 09:56 PM, Nishanth Menon wrote: > On 07/23/2013 02:20 AM, Tero Kristo wrote: >> OMAP 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. > > why not extend standard framework to support autoidle capable clocks OR > introduce our own clk node which depends on basic clocks? Was kind of easier this way. >> >> Signed-off-by: Tero Kristo >> --- >> arch/arm/mach-omap2/clock.c | 6 ++ >> drivers/clk/omap/Makefile | 2 +- >> drivers/clk/omap/autoidle.c | 130 >> +++++++++++++++++++++++++++++++++++++++++++ >> drivers/clk/omap/clk.c | 4 +- >> include/linux/clk/omap.h | 4 ++ >> 5 files changed, 143 insertions(+), 3 deletions(-) >> create mode 100644 drivers/clk/omap/autoidle.c > > I know it is getting a little stale, but anyways, device tree binding > missing. > >> >> diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c >> index 0c38ca9..669d4c4 100644 >> --- a/arch/arm/mach-omap2/clock.c >> +++ b/arch/arm/mach-omap2/clock.c > Not sure why, at this point in time we are going to calling drivers/clk > code. > >> @@ -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_omap_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_omap_clk_deny_autoidle_all(); >> + > > these are defined for CONFIG_OF.. anyways, without dt nodes (OMAP3 is > supposed to support non-DT boot even now), this would not work, would it? The lists are empty so the funcs do nothing. However, dropping CONFIG_OF would break these of course. Will figure out a fix for this. The calls are needed for the transition phase until we can move more clk stuff from mach-omap2 to drivers. > > >> return 0; >> } >> >> diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile >> index 4cad480..ca56700 100644 >> --- a/drivers/clk/omap/Makefile >> +++ b/drivers/clk/omap/Makefile >> @@ -1 +1 @@ >> -obj-y += clk.o dpll.o >> +obj-y += clk.o dpll.o autoidle.o >> diff --git a/drivers/clk/omap/autoidle.c b/drivers/clk/omap/autoidle.c >> new file mode 100644 >> index 0000000..6424cb2 >> --- /dev/null >> +++ b/drivers/clk/omap/autoidle.c >> @@ -0,0 +1,130 @@ >> +/* >> + * OMAP 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 >> +#include >> +#include >> +#include > at all of these required? I'll double check. > >> + >> +#ifdef CONFIG_OF >> +struct clk_omap_autoidle { >> + void __iomem *reg; >> + u8 shift; >> + u8 flags; >> + const char *name; >> + struct list_head node; >> +}; >> + >> +#define AUTOIDLE_LOW 0x1 >> + >> +static LIST_HEAD(autoidle_clks); >> + >> +static void omap_allow_autoidle(struct clk_omap_autoidle *clk) >> +{ >> + u32 val; >> + >> + val = readl(clk->reg); >> + >> + if (clk->flags & AUTOIDLE_LOW) >> + val &= ~(1 << clk->shift); >> + else >> + val |= (1 << clk->shift); >> + >> + writel(val, clk->reg); >> +} >> + >> +static void omap_deny_autoidle(struct clk_omap_autoidle *clk) >> +{ >> + u32 val; >> + >> + val = readl(clk->reg); >> + >> + if (clk->flags & AUTOIDLE_LOW) >> + val |= (1 << clk->shift); >> + else >> + val &= ~(1 << clk->shift); >> + >> + writel(val, clk->reg); >> +} >> + >> +void of_omap_clk_allow_autoidle_all(void) >> +{ >> + struct clk_omap_autoidle *c; >> + >> + list_for_each_entry(c, &autoidle_clks, node) >> + omap_allow_autoidle(c); >> +} >> + >> +void of_omap_clk_deny_autoidle_all(void) >> +{ >> + struct clk_omap_autoidle *c; >> + >> + list_for_each_entry(c, &autoidle_clks, node) >> + omap_deny_autoidle(c); >> +} >> + >> +static __init void of_omap_autoidle_setup(struct device_node *node) >> +{ >> + u32 shift; >> + void __iomem *reg; >> + struct clk_omap_autoidle *clk; >> + >> + if (of_property_read_u32(node, "ti,autoidle-shift", &shift)) >> + return; >> + >> + reg = of_iomap(node, 0); >> + >> + clk = kzalloc(sizeof(struct clk_omap_autoidle), GFP_KERNEL); >> + >> + if (!clk) { >> + pr_err("%s: kzalloc failed\n", __func__); >> + return; >> + } >> + >> + clk->shift = shift; >> + clk->name = node->name; >> + clk->reg = reg; >> + >> + if (of_property_read_bool(node, "ti,autoidle-low")) >> + clk->flags |= AUTOIDLE_LOW; >> + >> + list_add(&clk->node, &autoidle_clks); >> +} >> + >> +void __init of_omap_divider_setup(struct device_node *node) >> +{ >> + of_divider_clk_setup(node); >> + of_omap_autoidle_setup(node); >> +} >> +EXPORT_SYMBOL_GPL(of_omap_divider_setup); >> +CLK_OF_DECLARE(omap_autoidle_clock, "divider-clock", >> of_omap_divider_setup); > > This is overriding drivers/clk/clk-divider.c ? Yea, this declaration here should be removed. >> + >> +void __init of_omap_fixed_factor_setup(struct device_node *node) >> +{ >> + of_fixed_factor_clk_setup(node); >> + of_omap_autoidle_setup(node); >> +} >> +EXPORT_SYMBOL_GPL(of_omap_fixed_factor_setup); >> +CLK_OF_DECLARE(omap_fixed_factor_clock, "fixed-factor-clock", >> + of_omap_fixed_factor_setup); > > This is overriding drivers/clk/clk-fixed-factor.c ? Ditto. >> + >> +#endif >> diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c >> index cd31a81..c149097 100644 >> --- a/drivers/clk/omap/clk.c >> +++ b/drivers/clk/omap/clk.c >> @@ -25,8 +25,8 @@ static const struct of_device_id clk_match[] = { >> {.compatible = "fixed-clock", .data = of_fixed_clk_setup, }, >> {.compatible = "mux-clock", .data = of_mux_clk_setup, }, >> {.compatible = "fixed-factor-clock", >> - .data = of_fixed_factor_clk_setup, }, >> - {.compatible = "divider-clock", .data = of_divider_clk_setup, }, >> + .data = of_omap_fixed_factor_setup, }, >> + {.compatible = "divider-clock", .data = of_omap_divider_setup, }, These should be enough for registration. >> {.compatible = "gate-clock", .data = of_gate_clk_setup, }, >> {.compatible = "ti,omap4-dpll-clock", .data = >> of_omap4_dpll_setup, }, >> {}, >> diff --git a/include/linux/clk/omap.h b/include/linux/clk/omap.h >> index c39e775..904bdad 100644 >> --- a/include/linux/clk/omap.h >> +++ b/include/linux/clk/omap.h >> @@ -192,5 +192,9 @@ extern const struct clk_hw_omap_ops >> clkhwops_omap4_dpllmx; >> int dt_omap_clk_init(void); >> extern void omap_dt_clocks_register(struct omap_dt_clk *oclks, int >> cnt); >> void of_omap4_dpll_setup(struct device_node *node); >> +void of_omap_fixed_factor_setup(struct device_node *node); >> +void of_omap_divider_setup(struct device_node *node); >> +void of_omap_clk_allow_autoidle_all(void); >> +void of_omap_clk_deny_autoidle_all(void); >> >> #endif >> > > I personally dont prefer the fact that divider-clock and > fixed-rate-clock now has double meaning - building a multi-arch kernel > for example, this can wreak havoc. standard definitions should not be > monkeyed around with thus if avoidable, and in this case, very much > avoidable. > > just my 2 cents. Yea, as described before, I'll change the code not to make a global override, instead, just make omap specific override which parses the extra params. That sound better or still see some issues with that? -Tero