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

[Michael Turquette <mturquette@baylibre.com>]

Quoting Tero Kristo (2016-01-04 11:15:36)
> On 01/04/2016 06:37 PM, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [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 lo=
ops
> >>> take anything from 0...1500ns, most typically consuming some 400-600n=
s. 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 wo=
rks
> >>> 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 disable=
d?
> =

> 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 clkc=
trl
> > 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