From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 Sender: geert.uytterhoeven@gmail.com In-Reply-To: <568A20E5.6040005@ti.com> References: <1450447141-29936-1-git-send-email-t-kristo@ti.com> <1450447141-29936-7-git-send-email-t-kristo@ti.com> <20160101054815.21738.79820@quark.deferred.io> <568A20E5.6040005@ti.com> Date: Mon, 4 Jan 2016 11:21:04 +0100 Message-ID: Subject: Re: [RFC 6/9] clk: ti: add support for omap4 module clocks From: Geert Uytterhoeven To: Tero Kristo Cc: Michael Turquette , "linux-omap@vger.kernel.org" , linux-clk , Tony Lindgren , Stephen Boyd , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 List-ID: Hi Tero, On Mon, Jan 4, 2016 at 8:36 AM, Tero Kristo wrote: > On 01/01/2016 07:48 AM, Michael Turquette wrote: >> Quoting Tero Kristo (2015-12-18 05:58:58) >>> +static int _omap4_hwmod_clk_enable(struct clk_hw *hw) >>> +{ >>> + struct clk_hw_omap *clk =3D to_clk_hw_omap(hw); >>> + u32 val; >>> + int timeout =3D 0; >>> + int ret; >>> + >>> + if (!clk->enable_bit) >>> + return 0; >>> + >>> + if (clk->clkdm) { >>> + ret =3D ti_clk_ll_ops->clkdm_clk_enable(clk->clkdm, >>> hw->clk); >>> + if (ret) { >>> + WARN(1, >>> + "%s: could not enable %s's clockdomain %s: >>> %d\n", >>> + __func__, clk_hw_get_name(hw), >>> + clk->clkdm_name, ret); >>> + return ret; >>> + } >>> + } >>> + >>> + val =3D ti_clk_ll_ops->clk_readl(clk->enable_reg); >>> + >>> + val &=3D ~OMAP4_MODULEMODE_MASK; >>> + val |=3D clk->enable_bit; >>> + >>> + ti_clk_ll_ops->clk_writel(val, clk->enable_reg); >>> + >>> + /* Wait until module is enabled */ >>> + while (!_omap4_is_ready(val)) { >>> + udelay(1); >> >> This should really be a .prepare callback if you plan to keep the delays >> in there. > > If this is changed to a .prepare, then all OMAP power management is > effectively ruined as all clocks are going to be enabled all the time. hw= mod > core doesn't support .prepare/.enable at the moment that well, and changi= ng > that is going to be a big burden (educated guess, haven't checked this > yet)... The call chain that comes here is: > > device driver -> pm_runtime -> hwmod_core -> hwmod_clk_enable / disable. > > The delay within this function should usually be pretty short, just to wa= it > that the module comes up from idle. Does it take multiple =C2=B5s? Perhaps even one =C2=B5s is much longer than= needed? > I recall the discussions regarding the udelays within clk_enable/disable > calls, but what is the preferred approach then? Typically clk_enable/disa= ble > just becomes a NOP if it is not allowed to wait for hardware to complete > transitioning before exiting the function. FWIW, there are small loops with just a cpu_relax() in various clock driver= s under drivers/clk/shmobile/. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k= .org In personal conversations with technical people, I call myself a hacker. Bu= t when I'm talking to journalists I just say "programmer" or something like t= hat. -- Linus Torvalds From mboxrd@z Thu Jan 1 00:00:00 1970 From: Geert Uytterhoeven Subject: Re: [RFC 6/9] clk: ti: add support for omap4 module clocks Date: Mon, 4 Jan 2016 11:21:04 +0100 Message-ID: References: <1450447141-29936-1-git-send-email-t-kristo@ti.com> <1450447141-29936-7-git-send-email-t-kristo@ti.com> <20160101054815.21738.79820@quark.deferred.io> <568A20E5.6040005@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <568A20E5.6040005@ti.com> Sender: linux-clk-owner@vger.kernel.org To: Tero Kristo Cc: Michael Turquette , "linux-omap@vger.kernel.org" , linux-clk , Tony Lindgren , Stephen Boyd , "linux-arm-kernel@lists.infradead.org" List-Id: linux-omap@vger.kernel.org Hi Tero, On Mon, Jan 4, 2016 at 8:36 AM, Tero Kristo wrote: > On 01/01/2016 07:48 AM, Michael Turquette wrote: >> Quoting Tero Kristo (2015-12-18 05:58:58) >>> +static int _omap4_hwmod_clk_enable(struct clk_hw *hw) >>> +{ >>> + struct clk_hw_omap *clk =3D to_clk_hw_omap(hw); >>> + u32 val; >>> + int timeout =3D 0; >>> + int ret; >>> + >>> + if (!clk->enable_bit) >>> + return 0; >>> + >>> + if (clk->clkdm) { >>> + ret =3D ti_clk_ll_ops->clkdm_clk_enable(clk->clkdm, >>> hw->clk); >>> + if (ret) { >>> + WARN(1, >>> + "%s: could not enable %s's clockdomain= %s: >>> %d\n", >>> + __func__, clk_hw_get_name(hw), >>> + clk->clkdm_name, ret); >>> + return ret; >>> + } >>> + } >>> + >>> + val =3D ti_clk_ll_ops->clk_readl(clk->enable_reg); >>> + >>> + val &=3D ~OMAP4_MODULEMODE_MASK; >>> + val |=3D clk->enable_bit; >>> + >>> + ti_clk_ll_ops->clk_writel(val, clk->enable_reg); >>> + >>> + /* Wait until module is enabled */ >>> + while (!_omap4_is_ready(val)) { >>> + udelay(1); >> >> This should really be a .prepare callback if you plan to keep the de= lays >> in there. > > If this is changed to a .prepare, then all OMAP power management is > effectively ruined as all clocks are going to be enabled all the time= =2E hwmod > core doesn't support .prepare/.enable at the moment that well, and ch= anging > that is going to be a big burden (educated guess, haven't checked thi= s > yet)... The call chain that comes here is: > > device driver -> pm_runtime -> hwmod_core -> hwmod_clk_enable / disab= le. > > The delay within this function should usually be pretty short, just t= o wait > that the module comes up from idle. Does it take multiple =C2=B5s? Perhaps even one =C2=B5s is much longer = than needed? > I recall the discussions regarding the udelays within clk_enable/disa= ble > calls, but what is the preferred approach then? Typically clk_enable/= disable > just becomes a NOP if it is not allowed to wait for hardware to compl= ete > transitioning before exiting the function. =46WIW, there are small loops with just a cpu_relax() in various clock = drivers under drivers/clk/shmobile/. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-= m68k.org In personal conversations with technical people, I call myself a hacker= =2E But when I'm talking to journalists I just say "programmer" or something li= ke that. -- Linus Torvalds From mboxrd@z Thu Jan 1 00:00:00 1970 From: geert@linux-m68k.org (Geert Uytterhoeven) Date: Mon, 4 Jan 2016 11:21:04 +0100 Subject: [RFC 6/9] clk: ti: add support for omap4 module clocks In-Reply-To: <568A20E5.6040005@ti.com> References: <1450447141-29936-1-git-send-email-t-kristo@ti.com> <1450447141-29936-7-git-send-email-t-kristo@ti.com> <20160101054815.21738.79820@quark.deferred.io> <568A20E5.6040005@ti.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Tero, On Mon, Jan 4, 2016 at 8:36 AM, Tero Kristo wrote: > On 01/01/2016 07:48 AM, Michael Turquette wrote: >> Quoting Tero Kristo (2015-12-18 05:58:58) >>> +static int _omap4_hwmod_clk_enable(struct clk_hw *hw) >>> +{ >>> + struct clk_hw_omap *clk = to_clk_hw_omap(hw); >>> + u32 val; >>> + int timeout = 0; >>> + int ret; >>> + >>> + if (!clk->enable_bit) >>> + return 0; >>> + >>> + if (clk->clkdm) { >>> + ret = ti_clk_ll_ops->clkdm_clk_enable(clk->clkdm, >>> hw->clk); >>> + if (ret) { >>> + WARN(1, >>> + "%s: could not enable %s's clockdomain %s: >>> %d\n", >>> + __func__, clk_hw_get_name(hw), >>> + clk->clkdm_name, ret); >>> + return ret; >>> + } >>> + } >>> + >>> + val = ti_clk_ll_ops->clk_readl(clk->enable_reg); >>> + >>> + val &= ~OMAP4_MODULEMODE_MASK; >>> + val |= clk->enable_bit; >>> + >>> + ti_clk_ll_ops->clk_writel(val, clk->enable_reg); >>> + >>> + /* Wait until module is enabled */ >>> + while (!_omap4_is_ready(val)) { >>> + udelay(1); >> >> This should really be a .prepare callback if you plan to keep the delays >> in there. > > If this is changed to a .prepare, then all OMAP power management is > effectively ruined as all clocks are going to be enabled all the time. hwmod > core doesn't support .prepare/.enable at the moment that well, and changing > that is going to be a big burden (educated guess, haven't checked this > yet)... The call chain that comes here is: > > device driver -> pm_runtime -> hwmod_core -> hwmod_clk_enable / disable. > > The delay within this function should usually be pretty short, just to wait > that the module comes up from idle. Does it take multiple ?s? Perhaps even one ?s is much longer than needed? > I recall the discussions regarding the udelays within clk_enable/disable > calls, but what is the preferred approach then? Typically clk_enable/disable > just becomes a NOP if it is not allowed to wait for hardware to complete > transitioning before exiting the function. FWIW, there are small loops with just a cpu_relax() in various clock drivers under drivers/clk/shmobile/. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds