From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752782AbdBJLvg (ORCPT ); Fri, 10 Feb 2017 06:51:36 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:26819 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752084AbdBJLvd (ORCPT ); Fri, 10 Feb 2017 06:51:33 -0500 X-AuditID: cbfec7f1-f793f6d000007796-d3-589da9406045 Subject: Re: [PATCH v8 3/3] dmaengine: pl330: Don't require irq-safe runtime PM To: Vinod Koul , "Rafael J. Wysocki" , Ulf Hansson Cc: linux-samsung-soc@vger.kernel.org, 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: Fri, 10 Feb 2017 12:51:26 +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: <20170210045004.GN19244@localhost> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0hTYRiA+c5tZ9KJ05r1opaw8kdWXrpxMDGj0PMzg1CitJEHHV6KHbVM qxFZutJsWpooWelS0y7LIlRMp2yF1cxURoZCYqW5zCRSc5rzLPDf8/I9fC/Px0fjijbSi9ak pgnaVHWyivIgnltmbFvDaytiguoagjinwSLjnpQ+IrkHN5wkZxgqJDib7bGMqyq6TXKm4X6S +9BUTnFT+Z2IK7W1Ytybrh6SszZEc6MFL4lwhv87a0D8vZZRjDfV5VH8p/4Win9adZ6vGjeT fGPfZYIvaKxD/JRp/QH5YY/QeCFZkyFoA8OOeSR2DFnRyUnV6ZrfJkyHvnrrEU0DuwPeTmj0 SL6Ia6B78BGlRx60gq1G0Pv6KikNUwi6LcWYZO0Ac/eE2zIiGHF+kEnDVwQX5oyEy1rNRsHH uR7KtULJZkDZdKzLwdnvGBivz5Iuh2KDQe/QUy5m2DD4UW3HXUywfjBwz45c7MkegXzTTVxy VsF00eDS/XI2EHSFd5ccnA2BL/M5pMS+8LTegbuWAeuQwbOpT5jUuQ5MbbhUsB/am0uQxKth zNook9gH8nLb3ZXXFmNyNktciuCdg5F4N3RY37t3rQTD8xJcup6B3EsKSeGhqdJMSLwX+kbu uN9Hh8Hwz4t4IfItW5ZTtiyhbFlCJcLrkFJIF1MSBHFbgKhOEdNTEwKOn0gxocV/1TVvnXyB Jl6FmBFLI9UK5lt2eYyCVGeImSlmBDSuUjJJ1RUxCiZenXlG0J6I06YnC6IZedOEai3TUtkb rWAT1GlCkiCcFLT/TzFa7qVDBvnR7ZkLPvY92QOeu7ZhpZba+zNzncWHQq2/ohc22seD2j77 jUUMFf6xZTplGmOc95XQqvDIGmY+2PqwfqA5qndfpc7h6TAKYvGY7knuqWiLPWxh50iof5b8 VlOWXRu5qaH5/hlrduzBTjGqeYtFmfZsIqJ1Qw/bdi7xLFK0qAgxUR3sj2tF9T/NeBoLUwMA AA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrMIsWRmVeSWpSXmKPExsVy+t/xK7o9K+dGGPz/J2Lxd9IxdouNM9az Wqye+pfVYtL9CSwW589vYLdYMnk+q8Wmx9dYLS7vmsNm8bn3CKPFjPP7mCzOnL7EanF8bbjF y779LA68Hr9/TWL0WLznJZPHplWdbB53ru1h89i8pN5jyZtDrB5brrazePRtWcXo8XmTXABn lJtNRmpiSmqRQmpecn5KZl66rVJoiJuuhZJCXmJuqq1ShK5vSJCSQlliTimQZ2SABhycA9yD lfTtEtwyDt8/zljwUalixddNTA2Mz6W7GDk5JARMJA5deM8GYYtJXLi3Hsjm4hASWMIo8XHB UyYI5zmjxOkH68CqhAX8JSZ92M4KYosIlEncXXGMBaKogUni/OKTYO3MAq+ZJGau/M8IUsUm YCjR9bYLrJtXwE7i3dIbzCA2i4CqxO3FN8BqRAViJPb232eCqBGU+DH5HguIzSmgL9EwYRFY DbOAmcSXl4dZIWx5ic1r3jJPYBSYhaRlFpKyWUjKFjAyr2IUSS0tzk3PLTbUK07MLS7NS9dL zs/dxAiM4G3Hfm7ewXhpY/AhRgEORiUe3glVcyKEWBPLiitzDzFKcDArifBmL50bIcSbklhZ lVqUH19UmpNafIjRFOiJicxSosn5wOSSVxJvaGJobmloZGxhYW5kpCTOW/LhSriQQHpiSWp2 ampBahFMHxMHp1QD48SiYFXx3abijEn5DVNvdK+LU+YPvlMScEe7rypO4UiH+L1Z/07fkEyo PcfVLe78ZnXx0+3pd7Wm/zmanunA9Kf+tfK6/SfOn599dYKuAP85iUsvN3itCbRuKgx+5f/f 0d3uzz32p9tPm38K1ubMCf+7rdC/Qm7CdnnRndcnm9YYONgYW56wUWIpzkg01GIuKk4EAGD2 mND2AgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170210115128eucas1p2ed99e8ff1c5a0883a16c6d3ac5dd2852 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 Vinod, 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. > 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. > 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. 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. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland