From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752729AbdBMLpd (ORCPT ); Mon, 13 Feb 2017 06:45:33 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:9582 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751652AbdBMLpa (ORCPT ); Mon, 13 Feb 2017 06:45:30 -0500 X-AuditID: cbfec7f1-f793f6d000007796-bf-58a19c55d9c9 Subject: Re: [PATCH v8 3/3] dmaengine: pl330: Don't require irq-safe runtime PM To: Ulf Hansson , Vinod Koul Cc: "Rafael J. Wysocki" , linux-samsung-soc , dmaengine@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" , "linux-pm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Krzysztof Kozlowski , Bartlomiej Zolnierkiewicz , Lars-Peter Clausen , Arnd Bergmann , Inki Dae From: Marek Szyprowski Message-id: Date: Mon, 13 Feb 2017 12:45:23 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02Se0hTYRjG+3bO2c5Gq+O0fLFUGkRQpJkGBwvLUDoZkSWmiVAjDyo5tR0V 7YKXvA7mZKKZmJfSCK3UOUTMUpe5wFtaSohZKZWLLMz7tdyOgf/93u973ud9n4+PxGTthAMZ GR3HqqIVUXKhBG/sXOw9GFhSEXwoXyunV3WdIrq+qJagawpWCVr3KQ+n+/rqRHRlfhlB68eH CPpdc4mQntZ0ILqo76WA7u4aIGjT0yDanNuKn5Ayy0s6xDxsMQsYfXWOkBkZahEyDZXJTOVP I8EYBrNwJtdQjZhpvZO/OERyLIyNikxgVa5eVyQRpakTwljtycR2nRmloCl3NRKTQHnARPaa kOed8Ha0dp0lpIyqQvB3bRzji2kE8yYNUiPS2tFaco4/f4RAm5eD+OL7eoemibBY2VLnYXhl wGprR52B9O5CkUWEUQU4NN3Ps4qElBuoJ9VWkZTygoKJdMzCOLUXUv/UCSy8gwoFjb4Q4zU2 sJA/iltYTAVApnbK6oNRnvBtLWODnaHhyaR1baBmRVBX3b6xtiPo2zA+pw/Uv+4X8WwLP0yG Dd4NOdntAp61CNIyDvBchKB3UsrzUXhl6t+YtQ10jXcx3l4K2ZkyXsJAc7kR59kbBr9WiPgH WhFAmmFMkIecizfFKd4UoXhThHKEVSM7Np5ThrPcYRdOoeTio8NdrsYo9Wj9Y3Wtmaaa0O83 nkZEkUi+VZqSWR4sIxQJXJLSiIDE5HbSpIKKYJk0TJF0g1XFXFbFR7GcEe0icbm9tKX8fZCM ClfEsddYNpZV/b8VkGKHFJT0+Vfo1LM211S1z/bRS/42rtkfRpK192qGj8RIUkfOvvgSWN+z 4C8po8ds557rSos0XQ8uGGa4EPsxVq09rnQEj5uzK49zwkrN4qXbIwFmjZ977oJfz/ZbDqcm mGVfx9PXfeaqmrckrl5cGZ3p857fs/DxTpZvi1P9YsTQvsYOjRznIhRu+zEVp/gHHadhilQD AAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrEIsWRmVeSWpSXmKPExsVy+t/xq7qL5yyMMNjZLGfxd9IxdouNM9az Wqye+pfVYtL9CSwW589vYLdYMnk+q8Wmx9dYLS7vmsNm8bn3CKPFjPP7mCzOnL7EanF8bbjF y779LA68Hr9/TWL0WLznJZPHplWdbB53ru1h89i8pN5jyZtDrB5brrazePRtWcXo8XmTXABn lJtNRmpiSmqRQmpecn5KZl66rVJoiJuuhZJCXmJuqq1ShK5vSJCSQlliTimQZ2SABhycA9yD lfTtEtwy5jW+YCvod6o4OOklYwPjR+MuRg4OCQETif1z/LsYOYFMMYkL99azdTFycQgJLGGU +NXyghXCec4ocf7LG0aQKmEBf4lJH7azgtgiAt4SLWemsUMU/WGSOPzxNlgHs8B0Fon305cw gVSxCRhKdL3tYgOxeQXsJKa+aGEGsVkEVCUaP20AqxEViJHY23+fCaJGUOLH5HssIDanQLDE 8bU/wGxmATOJLy8Ps0LY8hKb17xlnsAoMAtJyywkZbOQlC1gZF7FKJJaWpybnltsqFecmFtc mpeul5yfu4kRGL/bjv3cvIPx0sbgQ4wCHIxKPLwNbQsihFgTy4orcw8xSnAwK4nwusxeGCHE m5JYWZValB9fVJqTWnyI0RToiYnMUqLJ+cDUklcSb2hiaG5paGRsYWFuZKQkzlvy4Uq4kEB6 YklqdmpqQWoRTB8TB6dUA6PC/TJWBpZ1VzSWLl3qdVF10wpVJz6LDTd7csTXC1+W/axaVM92 KrlkTtyLFylyNuXrGS31dW37dvT4SG76Yeu6ZHm5sNC7/WVaLde23mFVnBe3XL5rhj9D7N4b Ez1uSM2zksy9ssnx7g7jdYdWOO7kjZz+5hyzpfH+65b6Me9/J/6J3rnker8SS3FGoqEWc1Fx IgBjNckx9QIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170213114525eucas1p2bc72f538e4fbdc233aa4f82fc56d62d8 X-Msg-Generator: CA X-Sender-IP: 182.198.249.179 X-Local-Sender: =?UTF-8?B?TWFyZWsgU3p5cHJvd3NraRtTUlBPTC1LZXJuZWwgKFRQKRs=?= =?UTF-8?B?7IK87ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?TWFyZWsgU3p5cHJvd3NraRtTUlBPTC1LZXJuZWwgKFRQKRtT?= =?UTF-8?B?YW1zdW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170209142309eucas1p2b1277d96139eafc0d1dcc14145600476 X-RootMTR: 20170209142309eucas1p2b1277d96139eafc0d1dcc14145600476 References: <1486650171-20598-1-git-send-email-m.szyprowski@samsung.com> <1486650171-20598-4-git-send-email-m.szyprowski@samsung.com> <20170210045004.GN19244@localhost> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ulf, On 2017-02-10 14:57, Ulf Hansson wrote: > On 10 February 2017 at 12:51, Marek Szyprowski wrote: >> On 2017-02-10 05:50, Vinod Koul wrote: >>> On Thu, Feb 09, 2017 at 03:22:51PM +0100, Marek Szyprowski wrote: >>>> +static int pl330_set_slave(struct dma_chan *chan, struct device *slave) >>>> +{ >>>> + struct dma_pl330_chan *pch = to_pchan(chan); >>>> + struct pl330_dmac *pl330 = pch->dmac; >>>> + int i; >>>> + >>>> + mutex_lock(&pl330->rpm_lock); >>>> + >>>> + for (i = 0; i < pl330->num_peripherals; i++) { >>>> + if (pl330->peripherals[i].chan.slave == slave && >>>> + pl330->peripherals[i].slave_link) { >>>> + pch->slave_link = >>>> pl330->peripherals[i].slave_link; >>>> + goto done; >>>> + } >>>> + } >>>> + >>>> + pch->slave_link = device_link_add(slave, pl330->ddma.dev, >>>> + DL_FLAG_PM_RUNTIME | >>>> DL_FLAG_RPM_ACTIVE); >>> So you are going to add the link on channel allocation and tear down on >>> the >>> freeup. >> >> Right. Channel allocation is typically done once per driver operation and it >> won't hurt system performance. >> >>> I am not sure I really like the idea here. >> >> Could you point what's wrong with it? >> >>> First, these thing shouldn't be handled in the drivers. These things >>> should >>> be set in core and each driver setting the links doesn't sound great to >>> me. >> >> Which core? And what's wrong with the device links? They have been >> introduced to >> model relations between devices that are behind the usual parent/child/bus >> topology. > I think Vinod mean the dmaengine core. Which also would make perfect > sense to me as it would benefit all dma drivers. > > The only related PM thing, that shall be the decision of the driver, > is whether it wants to enable runtime PM or not, during ->probe(). So do you want to create the links during the DMAengine driver probe? How do you plan to find all the client devices? Please note that you really want to create links to devices which will really use the DMA engine calls. Some client drivers might decide in runtime weather to use DMA engine or not, depending on other data. >>> Second, should the link be always there and we only mange the state? Here >>> it >>> seems that we have link being created and destroyed, so why not mark it >>> ACTIVE and DORMANT instead... >> >> Link state is managed by device core and should not be touched by the >> drivers. >> It is related to both provider and consumer drivers states (probed/not >> probed/etc). >> >> Second we would need to create those links first. The question is where to >> create them then. > Just to fill in, to me this is really also the key question. > > If we could set up the device link already at device initialization, > it should also be possible to avoid getting -EPROBE_DEFER for dma > client drivers when requesting their dma channels. At the first glance this sounds like an ultimate solution for all problems, but I don't think that device links can be used this way. If I get it right, you would like to create links on client device initialization, preferably somewhere in the kernel driver core. This will be handled somehow by a completely generic code, which will create a link each pair of devices, which are connected by a phandle. Is this what you meant? Please note that that time no driver for both client and provider are probed. IMHO that doesn't look like a right generic approach How that code will know get following information: 1. is it really needed to create a link for given device pair? 2. what link flags should it use? 3. what about circular dependencies? 4. what about runtime optional dependencies? 5. what about non-dt platforms? acpi? This looks like another newer ending story of "how can we avoid deferred probe in a generic way". IMHO we should first solve the problem of irq-safe runtime PM in DMA engine drivers first. I proposed how it can be done with device links. With no changes in the client API. Later if one decide to extend the client API in a way it will allow other runtime PM implementation - I see no problem to convert pl330 driver to the new approach, but for the time being - this would be the easiest way to get it really functional. >>> Lastly, looking at th description of the issue here, am perceiving (maybe >>> my >>> understanding is not quite right here) that you have an IP block in SoC >>> which has multiple things and share common stuff and doing right PM is a >>> challenge for you, right? >> >> Nope. Doing right PM in my SoC is not that complex and I would say it is >> rather >> typical for any embedded stuff. It works fine (in terms of the power >> consumption reduction) when all drivers simply properly manage their runtime >> PM state, thus if device is not in use, the state is set to suspended and >> finally, the power domain gets turned off. >> >> I've used device links for PM only because the current DMA engine API is >> simply insufficient to implement it in the other way. >> >> I want to let a power domain, which contains a few devices, among those a >> PL330 >> device, to get turned off when there is no activity. Handling power domain >> power >> on / off requires non-atomic context, what is typical for runtime pm calls. >> For >> that I need to have non-irq-safe runtime pm implemented for all devices that >> belongs to that domains. > Again, allow me to fill in. This issue exists for all ARM SoC which > has a dma controller residing in a PM domain. I think that is quite > many. > > Currently the only solution I have seen for this problem, but which I > really dislike. That is, each dma client driver requests/releases > their dma channel from their respective ->runtime_suspend|resume() > callbacks - then the dma driver can use the dma request/release hooks, > to do pm_runtime_get|put() which then becomes non-irq-safe. > >> The problem with PL330 driver is that it use irq-safe runtime pm, which like >> it >> was stated in the patch description doesn't bring much benefits. To switch >> to >> standard (non-irq-safe) runtime pm, the pm_runtime calls have to be done >> from >> a context which permits sleeping. The problem with DMA engine driver API is >> that >> most of its callbacks have to be IRQ-safe and frankly only >> device_{alloc,release}_chan_resources() what more or less maps to >> dma_request_chan()/dma_release_channel() and friends. There are DMA engine >> drivers which do runtime PM calls there (tegra20-apb-dma, sirf-dma, cppi41, >> rcar-dmac), but this is not really efficient. DMA engine clients usually >> allocate >> dma channel during their probe() and keep them for the whole driver life. In >> turn >> this very similar to calling pm_runtime_get() in the DMA engine driver >> probe(). >> The result of both approaches is that DMA engine device keeps its power >> domain >> enabled almost all the time. This problem is also mentioned in the DMA >> engine >> TODO list, you have pointed me yesterday. >> >> To avoid such situation that DMA engine driver blocks turning off the power >> domain and avoid changing DMA engine client API I came up with the device >> links >> pm based approach. I don't want to duplicate the description here, the >> details >> were in the patch description, however if you have any particular question >> about >> the details, let me know and I will try to clarify it more. > So besides solving the irq-safe issue for dma driver, using the > device-links has additionally two advantages. I already mentioned the > -EPROBE_DEFER issue above. Not really. IMHO device links can be properly established once both drivers are probed... > > The second thing, is the runtime/system PM relations we get for free > by using the links. In other words, the dma driver/core don't need to > care about dealing with pm_runtime_get|put() as that would be managed > by the dma client driver. IMHO there might be drivers which don't want to use device links based runtime PM in favor of irq-safe PM or something else. This should be really left to drivers. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland