From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753945AbcFGHKd (ORCPT ); Tue, 7 Jun 2016 03:10:33 -0400 Received: from mail-pf0-f195.google.com ([209.85.192.195]:33675 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753908AbcFGHKb (ORCPT ); Tue, 7 Jun 2016 03:10:31 -0400 Date: Tue, 7 Jun 2016 15:04:40 +0800 From: Dong Aisheng To: Thomas Gleixner Cc: Shawn Guo , Lucas Stach , Michael Turquette , Stephen Boyd , "linux-kernel@vger.kernel.org" , Stefan Agner , mingo@redhat.com, "kernel@pengutronix.de" , linux-clk@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH 1/2] clk: imx: do not sleep if IRQ's are still disabled Message-ID: <20160607070440.GA32573@shlinux2> References: <20160426012341.GB8870@tiger> <1461663072.7839.17.camel@pengutronix.de> <20160427015835.GE30692@tiger> <20160602145915.GA31124@shlinux2> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Date: Tue, 7 Jun 2016 15:04:40 +0800 From: Dong Aisheng To: Thomas Gleixner Cc: Shawn Guo , Lucas Stach , Michael Turquette , Stephen Boyd , "linux-kernel@vger.kernel.org" , Stefan Agner , mingo@redhat.com, "kernel@pengutronix.de" , linux-clk@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH 1/2] clk: imx: do not sleep if IRQ's are still disabled Message-ID: <20160607070440.GA32573@shlinux2> References: <20160426012341.GB8870@tiger> <1461663072.7839.17.camel@pengutronix.de> <20160427015835.GE30692@tiger> <20160602145915.GA31124@shlinux2> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: List-ID: 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: dongas86@gmail.com (Dong Aisheng) Date: Tue, 7 Jun 2016 15:04:40 +0800 Subject: [PATCH 1/2] clk: imx: do not sleep if IRQ's are still disabled In-Reply-To: References: <20160426012341.GB8870@tiger> <1461663072.7839.17.camel@pengutronix.de> <20160427015835.GE30692@tiger> <20160602145915.GA31124@shlinux2> Message-ID: <20160607070440.GA32573@shlinux2> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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