Re: [PATCH 1/2] clk: imx: do not sleep if IRQ's are still disabled

On Thu, 2 Jun 2016, Dong Aisheng wrote:

On Wed, Apr 27, 2016 at 12:15:00PM +0200, Thomas Gleixner wrote:

quoted

Calling a function which might sleep _BEFORE_ kernel_init() is wrong. Don't
try to work around such an issue by doing magic irq_disabled() checks and busy
loops. Fix the call site and be done with it.

IRQ chip and clocksource are also initialised before kernel_init()
which may call clk APIs like clk_prepare_enable().
We may not be able to guarantee those clocks are unsleepable.

e.g.
For IRQ chip, the arm,gic documents the optional clocks property in
Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt.
Although there seems no user currently, not sure there might be
a exception in the future.

For clocksource driver, it's quite common to call clk APIs to
enable and get clock rate which may also cause sleep.
e.g. ARM twd clock in arch/arm/kernel/smp_twd.c.

If we have to avoid the possible sleep caused by these clocks,
we may need to manually check it and change the related clocks
(e.g PLLs) from sleepable to busy loops.
But that is not a good solution and may also not stable when
clock varies.

It looks like not easy to find a generic solution for them.
What's your suggestion?

I think we should split the prepare callbacks up and move the handling logic
into the core.

clk_ops.prepare()	Legacy callback
clk_ops.prepare_hw()	Callback which writes to the hardware
clk_ops.prepare_done()	Callback which checks whether the prepare is completed

So now the core can do:

clk_prepare()
{
	/* Legacy code should go away */
	if (ops->prepare) {
	      ops->prepare();
	      return;
	}

	if (ops->prepare_hw)
	      ops->prepare_hw();

   	if (!ops->prepare_done())
		return;

	if (early_boot_or_suspend_resume()) {
		/*
		 * Busy loop when we can't schedule in early boot,
		 * suspend and resume.
		 */
		while (!ops->prepare_done())
		      ;
	} else {
		while (!ops->prepare_done())
		      usleep(clk->prepare_delay);
	}
}

As a side effect that allows us to remove the gazillion of

     	    while (!hw_ready)
	    	  usleep();

copies all over the place and we have a single point of control where we allow
the clocks to busy loop.

Toughts?

Thanks,

	tglx