Re: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Tero Kristo]

On 01/04/2016 06:37 PM, Tony Lindgren wrote:

* Russell King - ARM Linux  [160104 06:43]:

On Mon, Jan 04, 2016 at 03:27:57PM +0200, Tero Kristo wrote:

On 01/04/2016 12:21 PM, Geert Uytterhoeven wrote:

FWIW, there are small loops with just a cpu_relax() in various clock drivers
under drivers/clk/shmobile/.


Just did a quick profiling round, and the clk_enable/disable delay loops
take anything from 0...1500ns, most typically consuming some 400-600ns. So,
based on this, dropping the udelay and adding cpu_relax instead looks like a
good change. I just verified that changing the udelay to cpu_relax works
fine also, I just need to change the bail-out period to be something sane.


Was that profiling done with lockdep/lock debugging enabled or disabled?


omap2plus_defconfig, so lockdep was enabled. The profiling was done 
around the while {} block though, which should not have any locks within 
it (except for the SCM clocks, which may explain some of the higher 
latency numbers seen.)



And also the thing to check from the hw folks is what all do these clkctrl
bits really control. If they group together the OCP clock and an extra
functional clock for some devices the delays could be larger.


Does it matter really? The latencies are only imposed to the device in 
question, and lets face it, the same latencies are there already with 
the hwmod implementation. This series moves the implementation under 
clock driver with as less modifications as possible to avoid any problems.



In general, I think we need to get rid of pm_runtime_irq_safe usage to
allow clocks to sleep properly. The other option is to allow toggling
pm_runtime_irq_safe but that probably gets super messy.


That is something not to be done with this set though.

-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: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Geert Uytterhoeven]

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 -- ge...@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
--
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: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Michael Turquette]

Quoting Tero Kristo (2016-01-04 11:15:36)
> On 01/04/2016 06:37 PM, Tony Lindgren wrote:
> > * Russell King - ARM Linux  [160104 06:43]:
> >> On Mon, Jan 04, 2016 at 03:27:57PM +0200, Tero Kristo wrote:
> >>> On 01/04/2016 12:21 PM, Geert Uytterhoeven wrote:
>  FWIW, there are small loops with just a cpu_relax() in various clock 
>  drivers
>  under drivers/clk/shmobile/.
> >>>
> >>> Just did a quick profiling round, and the clk_enable/disable delay loops
> >>> take anything from 0...1500ns, most typically consuming some 400-600ns. 
> >>> So,
> >>> based on this, dropping the udelay and adding cpu_relax instead looks 
> >>> like a
> >>> good change. I just verified that changing the udelay to cpu_relax works
> >>> fine also, I just need to change the bail-out period to be something sane.
> >>
> >> Was that profiling done with lockdep/lock debugging enabled or disabled?
> 
> omap2plus_defconfig, so lockdep was enabled. The profiling was done 
> around the while {} block though, which should not have any locks within 
> it (except for the SCM clocks, which may explain some of the higher 
> latency numbers seen.)
> 
> > And also the thing to check from the hw folks is what all do these clkctrl
> > bits really control. If they group together the OCP clock and an extra
> > functional clock for some devices the delays could be larger.
> 
> Does it matter really? The latencies are only imposed to the device in 
> question, and lets face it, the same latencies are there already with 
> the hwmod implementation. This series moves the implementation under 
> clock driver with as less modifications as possible to avoid any problems.

So long as we can all convince ourselves that the flaw is not a flaw
then I'm OK with it. No bugs were ever introduced that way ;-)

But in fairness, we've had these delays in the .enable callbacks for a
while, so this patch does not introduce the regression. Furthermore it
does clean up some code that needs more work, and I don't want to delay
that.

I won't NACK the patch due to the delays, but it would be nice to
revisit it some day.

Regards,
Mike

> 
> > In general, I think we need to get rid of pm_runtime_irq_safe usage to
> > allow clocks to sleep properly. The other option is to allow toggling
> > pm_runtime_irq_safe but that probably gets super messy.
> 
> That is something not to be done with this set though.
> 
> -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: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Michael Turquette]

Quoting Tero Kristo (2016-01-03 23:36:05)
> On 01/01/2016 07:48 AM, Michael Turquette wrote:
> > Hi Tero,
> >
> > Quoting Tero Kristo (2015-12-18 05:58:58)
> >> Previously, hwmod core has been used for controlling the hwmod level
> >> clocks. This has certain drawbacks, like being unable to share the
> >> clocks for multiple users, missing usecounting and generally being
> >> totally incompatible with common clock framework.
> >>
> >> Add support for new clock type under the TI clock driver, which will
> >> be used to convert all the existing hwmdo clocks to. This helps to
> >> get rid of the clock related hwmod data from kernel and instead
> >> parsing this from DT.
> >
> > I'm really happy to see this series. Looks pretty good to me.
> >
> >> +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. 

Let's not ruin system PM.

> 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.

Right, and for calls to pm_runtime_get/put from process context it
should result in a call to clk_prepare_enable/clk_disable_unprepare. I
guess that change is hugely invasive from your statements above?

> 
> The delay within this function should usually be pretty short, just to 
> wait that the module comes up from idle.
> 
> I recall the discussions regarding the udelays within clk_enable/disable 
> calls, but what is the preferred approach then?

There are many cases where a clk only provides .{un}prepare ops and does
NOT provide any .{en,dis}able ops.

> Typically 
> clk_enable/disable just becomes a NOP

Yes, it becomes a NOP (though it is critical that drivers with knowledge
of this do not try to skip the step of calling clk_enable).

> if it is not allowed to wait for 
> hardware to complete transitioning before exiting the function.

The clk.h api clearly states that clk_prepare must be called and
complete before calling clk_enable. So if a clk only provides a .prepare
with delays but no .enable, and a consumer driver complies with that api
rule then we're guaranteed to have a toggling line when clk_enable
returns.

Regards,
Mike

> 
> -Tero
> 
> >
> > Regards,
> > Mike
> >
> 
--
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: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Tero Kristo]

On 01/04/2016 12:21 PM, Geert Uytterhoeven wrote:

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/.


Just did a quick profiling round, and the clk_enable/disable delay loops 
take anything from 0...1500ns, most typically consuming some 400-600ns. 
So, based on this, dropping the udelay and adding cpu_relax instead 
looks like a good change. I just verified that changing the udelay to 
cpu_relax works fine also, I just need to change the bail-out period to 
be something sane.


-Tero





Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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



--
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: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Russell King - ARM Linux]

On Mon, Jan 04, 2016 at 03:27:57PM +0200, Tero Kristo wrote:
> On 01/04/2016 12:21 PM, Geert Uytterhoeven wrote:
> >FWIW, there are small loops with just a cpu_relax() in various clock drivers
> >under drivers/clk/shmobile/.
> 
> Just did a quick profiling round, and the clk_enable/disable delay loops
> take anything from 0...1500ns, most typically consuming some 400-600ns. So,
> based on this, dropping the udelay and adding cpu_relax instead looks like a
> good change. I just verified that changing the udelay to cpu_relax works
> fine also, I just need to change the bail-out period to be something sane.

Was that profiling done with lockdep/lock debugging enabled or disabled?

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Tony Lindgren]

* Russell King - ARM Linux  [160104 06:43]:
> On Mon, Jan 04, 2016 at 03:27:57PM +0200, Tero Kristo wrote:
> > On 01/04/2016 12:21 PM, Geert Uytterhoeven wrote:
> > >FWIW, there are small loops with just a cpu_relax() in various clock 
> > >drivers
> > >under drivers/clk/shmobile/.
> > 
> > Just did a quick profiling round, and the clk_enable/disable delay loops
> > take anything from 0...1500ns, most typically consuming some 400-600ns. So,
> > based on this, dropping the udelay and adding cpu_relax instead looks like a
> > good change. I just verified that changing the udelay to cpu_relax works
> > fine also, I just need to change the bail-out period to be something sane.
> 
> Was that profiling done with lockdep/lock debugging enabled or disabled?

And also the thing to check from the hw folks is what all do these clkctrl
bits really control. If they group together the OCP clock and an extra
functional clock for some devices the delays could be larger.

In general, I think we need to get rid of pm_runtime_irq_safe usage to
allow clocks to sleep properly. The other option is to allow toggling
pm_runtime_irq_safe but that probably gets super messy.

Regards,

Tony
--
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: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Tero Kristo]

On 01/01/2016 07:48 AM, Michael Turquette wrote:

Hi Tero,

Quoting Tero Kristo (2015-12-18 05:58:58)

Previously, hwmod core has been used for controlling the hwmod level
clocks. This has certain drawbacks, like being unable to share the
clocks for multiple users, missing usecounting and generally being
totally incompatible with common clock framework.

Add support for new clock type under the TI clock driver, which will
be used to convert all the existing hwmdo clocks to. This helps to
get rid of the clock related hwmod data from kernel and instead
parsing this from DT.


I'm really happy to see this series. Looks pretty good to me.


+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.


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.


-Tero



Regards,
Mike



--
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: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Michael Turquette]

Hi Tero,

Quoting Tero Kristo (2015-12-18 05:58:58)
> Previously, hwmod core has been used for controlling the hwmod level
> clocks. This has certain drawbacks, like being unable to share the
> clocks for multiple users, missing usecounting and generally being
> totally incompatible with common clock framework.
> 
> Add support for new clock type under the TI clock driver, which will
> be used to convert all the existing hwmdo clocks to. This helps to
> get rid of the clock related hwmod data from kernel and instead
> parsing this from DT.

I'm really happy to see this series. Looks pretty good to me.

> +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.

Regards,
Mike
--
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

[RFC 6/9] clk: ti: add support for omap4 module clocks

[Tero Kristo]

Previously, hwmod core has been used for controlling the hwmod level
clocks. This has certain drawbacks, like being unable to share the
clocks for multiple users, missing usecounting and generally being
totally incompatible with common clock framework.

Add support for new clock type under the TI clock driver, which will
be used to convert all the existing hwmdo clocks to. This helps to
get rid of the clock related hwmod data from kernel and instead
parsing this from DT.

Signed-off-by: Tero Kristo 
---
 drivers/clk/ti/Makefile   |3 +-
 drivers/clk/ti/clkt_mod.c |  351 +
 include/linux/clk/ti.h|2 +
 3 files changed, 355 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/ti/clkt_mod.c

diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
index d4ac960..d1b9d41 100644
--- a/drivers/clk/ti/Makefile
+++ b/drivers/clk/ti/Makefile
@@ -1,7 +1,8 @@
 obj-y  += clk.o autoidle.o clockdomain.o
 clk-common = dpll.o composite.o divider.o gate.o \
  fixed-factor.o mux.o apll.o \
- clkt_dpll.o clkt_iclk.o clkt_dflt.o
+ clkt_dpll.o clkt_iclk.o clkt_dflt.o \
+ clkt_mod.o
 obj-$(CONFIG_SOC_AM33XX)   += $(clk-common) clk-33xx.o dpll3xxx.o
 obj-$(CONFIG_SOC_TI81XX)   += $(clk-common) fapll.o clk-814x.o 
clk-816x.o
 obj-$(CONFIG_ARCH_OMAP2)   += $(clk-common) interface.o clk-2xxx.o
diff --git a/drivers/clk/ti/clkt_mod.c b/drivers/clk/ti/clkt_mod.c
new file mode 100644
index 000..186d5f7
--- /dev/null
+++ b/drivers/clk/ti/clkt_mod.c
@@ -0,0 +1,351 @@
+/*
+ * OMAP hardware module clock support
+ *
+ * Copyright (C) 2015 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 "clock.h"
+
+#undef pr_fmt
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#define OMAP4_MODULEMODE_MASK  0x3
+
+#define MODULEMODE_HWCTRL  0x1
+#define MODULEMODE_SWCTRL  0x2
+
+#define OMAP4_IDLEST_MASK  (0x3 << 16)
+#define OMAP4_IDLEST_SHIFT 16
+
+#define CLKCTRL_IDLEST_FUNCTIONAL  0x0
+#define CLKCTRL_IDLEST_INTERFACE_IDLE  0x2
+#define CLKCTRL_IDLEST_DISABLED0x3
+
+#define OMAP4_MAX_MODULE_READY_TIME2000
+#define OMAP4_MAX_MODULE_DISABLE_TIME  5000
+
+static u32 _omap4_idlest(u32 val)
+{
+   val &= OMAP4_IDLEST_MASK;
+   val >>= OMAP4_IDLEST_SHIFT;
+
+   return val;
+}
+
+static bool _omap4_is_idle(u32 val)
+{
+   val = _omap4_idlest(val);
+
+   return val == CLKCTRL_IDLEST_DISABLED;
+}
+
+static bool _omap4_is_ready(u32 val)
+{
+   val = _omap4_idlest(val);
+
+   return val == CLKCTRL_IDLEST_FUNCTIONAL ||
+  val == CLKCTRL_IDLEST_INTERFACE_IDLE;
+}
+
+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);
+   timeout++;
+   if (timeout > OMAP4_MAX_MODULE_READY_TIME) {
+   pr_err("%s: failed to enable\n", clk_hw_get_name(hw));
+   return -EBUSY;
+   }
+   val = ti_clk_ll_ops->clk_readl(clk->enable_reg);
+   }
+
+   return 0;
+}
+
+static void _omap4_hwmod_clk_disable(struct clk_hw *hw)
+{
+   struct clk_hw_omap *clk = to_clk_hw_omap(hw);
+   u32 val;
+   int timeout = 0;
+
+   if (!clk->enable_bit)
+   return;
+
+   val = 

Re: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Tony Lindgren]

* Tero Kristo  [151218 05:57]:
> Previously, hwmod core has been used for controlling the hwmod level
> clocks. This has certain drawbacks, like being unable to share the
> clocks for multiple users, missing usecounting and generally being
> totally incompatible with common clock framework.
> 
> Add support for new clock type under the TI clock driver, which will
> be used to convert all the existing hwmod clocks to. This helps to
> get rid of the clock related hwmod data from kernel and instead
> parsing this from DT.

Good to see this happening, few comments on what I've noticed so
far that should be considered.

We should be able to do a generic "ti,mux-div-gate" clock and use
it for the clkctrl too I think. See the suggested binding elsewhere
in this thread. That would also work for the clkout register at least.
Maybe the "ti,mux-div-gate" clock needs a separate clkctrl type
compatible to know it needs to sleep to wait for the status bit.

Also, we already know these clkctrl register do have multiple clocks,
like the GPIO debounce and dividers for debugss clock. And we want
to get rid of the overlapping reg entries.

> +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);
> + timeout++;
> + if (timeout > OMAP4_MAX_MODULE_READY_TIME) {
> + pr_err("%s: failed to enable\n", clk_hw_get_name(hw));
> + return -EBUSY;
> + }
> + val = ti_clk_ll_ops->clk_readl(clk->enable_reg);
> + }
> +
> + return 0;
> +}

Clocks that need to wait to lock need to be set up with clk_prepare
instead of clk_enable as pointed out by TGLX recently. Also see the
"[PATCH] clk: ti: Fix FAPLL udelay in clk_enable with clk_prepare".

Not sure what all needs to be fixed in the hwmod code for that to
happen and PM to work, but that probably needs to be done first.

Regards,

Tony
--
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