From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krzysztof Kozlowski Subject: Re: [PATCH v4 4/4] dma: pl330: add Power Management support Date: Wed, 24 Sep 2014 12:09:23 +0200 Message-ID: <1411553363.9968.0.camel@AMDC1943> References: <1410857494-15936-1-git-send-email-k.kozlowski@samsung.com> <1410857494-15936-5-git-send-email-k.kozlowski@samsung.com> <1411034996.23919.9.camel@AMDC1943> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mailout1.w1.samsung.com ([210.118.77.11]:46043 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750940AbaIXKJ2 (ORCPT ); Wed, 24 Sep 2014 06:09:28 -0400 In-reply-to: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Ulf Hansson Cc: Russell King , Dan Williams , Vinod Koul , "linux-kernel@vger.kernel.org" , dmaengine@vger.kernel.org, Grant Likely , Lars-Peter Clausen , Michal Simek , "Rafael J. Wysocki" , Len Brown , Pavel Machek , Randy Dunlap , Alan Stern , "linux-doc@vger.kernel.org" , "linux-pm@vger.kernel.org" , Kyungmin Park , Marek Szyprowski , Bartlomiej Zolnierkiewicz On wto, 2014-09-23 at 17:39 +0200, Ulf Hansson wrote: > On 18 September 2014 12:09, Krzysztof Kozlowski wrote: > > On =C5=9Bro, 2014-09-17 at 20:42 +0200, Ulf Hansson wrote: > >> On 16 September 2014 10:51, Krzysztof Kozlowski wrote: > > > > [...] > > > >> > > >> > @@ -2585,6 +2620,34 @@ static int pl330_dma_device_slave_caps(st= ruct dma_chan *dchan, > >> > return 0; > >> > } > >> > > >> > +/* > >> > + * Assume that IRQ safe runtime PM is chosen in probe and amba = bus driver > >> > + * will only disable/enable the clock in runtime PM suspend/res= ume. > >> > + */ > >> > +static int __maybe_unused pl330_suspend(struct device *dev) > >> > +{ > >> > + struct amba_device *pcdev =3D to_amba_device(dev); > >> > + > >> > + if (!pm_runtime_suspended(dev)) > >> > + amba_pclk_disable(pcdev); > >> > >> I would suggest to use pm_runtime_force_suspend() instead of the a= bove. > > > > Sure, I can change it but... (see below) > > > >> > >> > + amba_pclk_unprepare(pcdev); > >> > + > >> > + return 0; > >> > +} > >> > + > >> > +static int __maybe_unused pl330_resume(struct device *dev) > >> > +{ > >> > + struct amba_device *pcdev =3D to_amba_device(dev); > >> > + > >> > + amba_pclk_prepare(pcdev); > >> > + if (!pm_runtime_suspended(dev)) > >> > + return amba_pclk_enable(pcdev); > >> > >> The above if statement could be replaced with pm_runtime_force_res= ume(). > > > > But that would lead to runtime resuming the device even when it is = not > > needed. We don't have to fully wakeup the device during resume oper= ation > > when the device was runtime suspended before suspend. >=20 > That's true - but that's not a big issue I believe. I think the below > problems are worse. :-) >=20 > 1) You don't disable runtime PM, thus not preventing the runtime PM > callbacks from being invoked. > 2) You don't update the runtime PM status, thus not reflecting the > actual state of the device. >=20 > Both the above may cause unbalanced clk_enable|disable calls. >=20 > Don't get me wrong, I certainly think the "don't do runtime PM resume > during system PM resume" is interesting to solve. But that it's a > common problem and should be supported by the runtime PM API somehow > instead. I have been thinking of extending the > pm_runtime_force_resume() API to support this, let's see when that > happens. :-) OK, I get the point. I'll fix this. > > > >> > >> Doing that, means the amba_pclk_enable|disable() don't need to be > >> exported from the AMBA bus header, but entirely handled by the AMB= A > >> bus itself. > >> > >> > + > >> > + return 0; > >> > +} > >> > + > >> > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume)= ; > >> > + > >> > static int > >> > pl330_probe(struct amba_device *adev, const struct amba_id *id) > >> > { > >> > @@ -2738,6 +2801,9 @@ pl330_probe(struct amba_device *adev, cons= t struct amba_id *id) > >> > pcfg->data_buf_dep, pcfg->data_bus_width / 8, pc= fg->num_chan, > >> > pcfg->num_peri, pcfg->num_events); > >> > > >> > >> You need pm_runtime_set_active() here as well. > > > > This is done by amba/bus.c. Do I have to do it again? >=20 > Ohh, you are right! No need to add it! >=20 > > > >> > >> > + pm_runtime_irq_safe(&adev->dev); > >> > + pm_runtime_put_noidle(&adev->dev); > >> > >> Why pm_runtime_put_noidle(), that seems like you might end up leav= ing > >> the device in active state - unless you get some request. Likely n= ot > >> what you want? > > > > Hmmm... I am sorry but I do not get the point here. It could be > > pm_runtime_put() as well because initially we start with counter > > incremented by amba/bus.c. >=20 > The "no_idle" version will prevent the device from going inactive, > unless a new cycle of pm_runtime_get() to pm_runtime_put() happens, > which there are no guarantees of. Right. Thanks for reviewing the code. I'll send the next version removing also the amba_pclk_enable|disable() macros. Best regards, Krzysztof >=20 > Kind regards > Uffe