From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934024AbcKIPHu (ORCPT ); Wed, 9 Nov 2016 10:07:50 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:16636 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932705AbcKIPHl (ORCPT ); Wed, 9 Nov 2016 10:07:41 -0500 X-AuditID: cbfec7f4-f791c6d000006eac-57-58233bb876af Subject: Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support To: "Luis R. Rodriguez" Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, linux-samsung-soc@vger.kernel.org, Joerg Roedel , Inki Dae , Kukjin Kim , =?UTF-8?Q?Krzysztof_Koz=c5=82owski?= , Bartlomiej Zolnierkiewicz , "Rafael J. Wysocki" , Mark Brown , Greg Kroah-Hartman , Tomeu Vizoso , Lukas Wunner , Kevin Hilman , Tobias Jakobi , Laurent Pinchart , Lars-Peter Clausen , Dmitry Torokhov , Grant Likely , Mauro Carvalho Chehab From: Marek Szyprowski Message-id: <9cf5829a-baac-9297-b231-17212d0195b1@samsung.com> Date: Wed, 09 Nov 2016 16:07:34 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-version: 1.0 In-reply-to: <20161108221437.GC13978@wotan.suse.de> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02SeUhUURTGufNWh8aeo9VJy2xEKpd24pqtFPUqiGihiECHepnkxoyGJZKU 6TiSWpM6aY27lgtTamYSmQsOomQ5ZppoiMuoNShMtPyhNONL8L/fPef77rnf4bKEPIV2Z8Mi YwRVpDJcQUvJ+va/3QENQd4XtuoHA/BLvZHCWSNjNG4tmkR4uuslje8WG2n88FsmiQuagnBq 3gsGW+b/MDhj9DuBx2tHJbhEl0/h1AclDDY3PqGx7X4bwvrudxI8afHA/ZnjCFdOxOGuzh4K J1fOUji9uoc+sJIfazZI+IahEsS/yR1i+DzNY4qvqUil+aanVQyv6y9H/Hw7w5f8aKH4ugy7 ou5zCsmn11Ug/u+UP2+r8eT1KfXUqeUXpXuuCOFhNwTVln0h0mudejMdXbUxTpNdTSSiMi8t YlngdkKjKVCLnOy4Ej4OG2kHy7lSBOONTloktbMNgcHwQbKof5a4RayXIbD0v2bEgwVBmj59 we3KHYXf3T2Ug904P7g7aZY4RAQ3RUPb3EfkaNDcNtBatQsGGbcPzOWGBQPJ+cAvWxHp4BXc JRh81EeIGhf4oxteqDtxO6BscJZxMMHthon5e5TI66C2yko4hgHXz8LXpixCfPZaqHlPiDEP Q1tpLyWyK0yb6hiR14BZl0aKnIHgzj0/kfUIPlhlIgdBq+nT/1nO8LA+5//1MtAky0UJDw2f xT0AdxAGXlkl4oJGJDBhyiEy0brcJXFyl0TIXRKhABEVyE2IVUeECuqdm9XKCHVsZOjmy1ER Ncj+PTvnTT8bUHH77hbEsUixTKbZ5H1BTilvqG9GtCBgCYWbrD3QXpJdUd68JaiiglWx4YK6 BXmwpGKV7G1B73k5F6qMEa4LQrSgWuxKWCf3RBRn3m+bdP7UfNv9yAnnL/mHTs8UGreb1r9y o0YHBH+57447MdVTZWQxfza+ck4XNXH76DP3mSCuT/N85vhQ/djq/F6Pc8PTrR1J8dfphBdd smy/QyaXQteQsb1ou2fSSZ/k4g7DscSAYO2ItPZMx4bY2ath57x++J19YNn1ODph7qKCVF9T bvMlVGrlPxCvbxGaAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA12Sa0hTYRjHeXd2zqY1Os1LLwYmZxTdpm7eXjFFCOZJCkSNxAI76fFSbtNN RYNCMnQO8oqms0xtaXnBa2Z9SJo3RMVyaVJo3nWaZWiWENKmBdLz6ff8+f0/PPDwMeEz3IEf p0hiVQomniKsuQPbfUZxmY8ozPVu5j7UXNKIo6LpOQJ1VS0BtDzYTKCMx40EKvicx0UVnT4o u6yJhxa3f/FQ7uwKhuZbZzlIX/gIR9n5eh4yvnpAoPV73QCVDL/moKXFw2g8bx6guoVUNDgw gqPMujUc5TSMEP729Nybcg7dMaEH9EvdBI8u05TidEttNkF3Pqzn0YXjNYDe7uXR+i8GnG7L NRtto1lcOqetFtBbptP0eosjXZLVjgcdCJediWWZKFblxCoilVFxihhf6mKoTIwoJwUjZ32p MPGF0GDKKYWJTzZvUtf/xv9sUEAI5eJ3VRY7UGIkEuqPp2qKG7B0UO2kBXw+JN3h03QXLbAy oz18O9lIaIE1X0jqAaxZWsZ3l0UAs/p7CYtlQwbAn8MjuIVtyVMwY8nIsbCQnObA1ilPSwEj vxFwZa5jRyJICdSuanfKAtIPGmvKd3IueRRurldxLWxHXoEfFmb+Ogfhr8LJndyKdIPVn9Z4 FsZIT7hh6sJ3+QhsrV/F8gCp21PR7dF0e7QKgNUCWzZZLY+Rq6XOakauTlbEOEcq5S3A/Cbt vVttHUD7NcQASD6g9gs0J0RhQpxJUafJDQDyMcpW0OttjgRRTNpNVqWMUCXHs2oD8DAfkY85 2EUqzU+nSIqQuEu8vCVSN4S8pFLqkKBo7f0lIRnDJLE3WDaBVf3rcfhWDukgzm24RXnOJqNy K3BsWhTePZ75wL5pItgeND8xeTPYjKNOtnGr4bxHfL5o41pl2eUhf5/5xKDbhlFNd/D965of 36NvVYttX4iefyQl+fsLflb0DM1Le0YTM/pzvANlYt93lSiamQ45JvNnVKbfY02aqb7oqogk P9nmndLNbrtiiquOZSQnMZWa+QP+TSRwPAMAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20161109150736eucas1p107aa77ffbfb98d90385e8c87c654ab57 X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 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: 20160929081240eucas1p231c1ab5c5936886cbec6bc3f5267c940 X-RootMTR: 20160929081240eucas1p231c1ab5c5936886cbec6bc3f5267c940 References: <1475136751-31340-1-git-send-email-m.szyprowski@samsung.com> <1475136751-31340-3-git-send-email-m.szyprowski@samsung.com> <20161006173756.GK3296@wotan.suse.de> <87aeb58b-bda0-93c5-e51d-ab9be0b9d518@samsung.com> <20161108221437.GC13978@wotan.suse.de> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Luis, On 2016-11-08 23:14, Luis R. Rodriguez wrote: > On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote: >> Hi Luis >> >> >> On 2016-10-06 19:37, Luis R. Rodriguez wrote: >>> On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote: >>>> This patch uses recently introduced device links to track the runtime pm >>>> state of the master's device. This way each SYSMMU controller is runtime >>>> activated when its master's device is active >>> instead of? >> instead of keeping SYSMMU controller runtime active all the time. > I thought Rafael's work was for suspend/resume, not for runtime suspend. > Is it for both ? Yes, it solves both problems, although the suspend/resume was easy to workaround just by using LATE_SLEEP_OPS. > Because as far as I can tell this was painted to help > with suspend/resume ? It also helped to remove the suspend/resume workaround. >>> BTW what is the master device of a SYSMMU? I have no clue about these >>> IOMMU devices here. >> Here is a more detailed description of IOMMU hardware I wrote a few days ago >> for Ulf: >> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1231006.html >> >> In short: there is a SYSMMU controller and its master device - a device, >> which performs DMA operations. That SYSMMU sits in between system memory >> and the master device, so it performs mapping of DMA addresses to physical >> memory addresses on each DMA operation. > So you seek a run time power optimization ? Or a fix on suspend? Or both? The main reason for using device links was to implement proper runtime power optimization. >>>> and can save/restore its state instead of being enabled all the time. >>> I take it this means currently even if the master device is disabled >>> (whatever that is) all SYSMMU controllers are kept enabled, is that right? >>> The issue here is this wastes power? Or what is the issue? >> Yes, the issue here is the fact that SYSMMU is kept active all the time, >> what in turn prevent the power domain for turning off even if master device >> doesn't do anything and is already suspended. This directly (some clocks >> enabled) and in-directly (leakage current) causes power looses. > Thanks for the confirmation so really the biggest concern here was run time PM. > >>>> This way SYSMMU controllers no >>>> longer prevents respective power domains to be turned off when master's >>>> device is not used. >>> So when the master device is idle we want to also remove power from the >>> controllers ? How much power does this save on a typical device in the >>> market BTW ? >> The main purpose of this patchset is to let power domains to be turned off, >> because with the current code all domains are all the time turned on, >> because >> SYSMMU controllers prevent them from turning them off. > I see.. I think the audio folks already addressed this with DAPM, but granted > this was for audio. Then I was also referred to the DRM / Audio component > framework, has this been looked into? v4l folks have v4l async stuff but > its not clear if that help with run time PM. I'm mentioning these given it'd be > silly to re-invent the wheel, additionally if we now have a generic solution > everyone can jump on board with there is quite a bit of work we can do to > dump a lot of old legacy crap. Right, probably some workarounds here and there can be removed. However components and v4l-async solutions are for resolving only probe and registration issues and they are some kind of pool for grouping devices and triggering special callback once all requested devices in the pool have probed. >> If you want I can measure the power consumption of the idle board with all >> domains enabled and disabled if you want to see the numbers. On the other >> board >> disabling most power domains in idle state (the clocks were already >> disabled) >> gave me about 20mA savings (at 3.7V), what is a significant value for the >> battery powered device. > Thanks, this means nothing to me, however it would be value-add to the commit log > as anyone reviewing this can understand what the goal / savings was for exactly. > >>>> Signed-off-by: Marek Szyprowski >>>> --- >>>> drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++------------------------- >>>> 1 file changed, 94 insertions(+), 131 deletions(-) >>> I'm reviewing the device link patches now but since this is a demo of >>> use of that I'll note the changes here are pretty large and it makes >>> it terribly difficult for review. Is there any way this patch can be split >>> up in to logical atomic pieces that only do one task upon change ? >> I will try to split it a bit, but I cannot promise that much can be done >> to improve readability for someone not very familiar with the driver >> internals. > I've heard this before, I don't buy it but lets see! Somehow I managed to split this all-in-one patch into several smaller changes in v5 and v6 was posted yesterday ago. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland