On Mon, Jun 06, 2016 at 03:20:03PM +0200, Thomas Gleixner wrote:
> On Thu, 2 Jun 2016, Dong Aisheng wrote:
> > On Wed, Apr 27, 2016 at 12:15:00PM +0200, Thomas Gleixner wrote:
> > > 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?
>
Great, that looks like a possible solution.
If we doing this way there's one more question that
should we do it for other clk APIs hold by prepare_lock which
all may sleep too in theory?
e.g. clk_{set|get}_rate/clk_{set|get}_parent. (possible more)
(clk_recalc_rate/clk_round_rate may also need be considered due to
it may be called within above function.)
And it seems many exist platforms doing that work in
CLK_OF_DECLARE init function in time_init().
e.g.
drivers/clk/imx/clk-imx6ul.c
drivers/clk/rockchip/clk-rk3188.c
drivers/clk/ti/clk-44xx.c
...
Then it may need introduce a lot changes and increase many new core APIs.
Is that a problem?
> Thanks,
>
> tglx
Regards
Dong Aisheng
