From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jarkko Nikula Subject: Re: [PATCH] i2c: designware: Fix system suspend Date: Thu, 10 Aug 2017 13:25:33 +0300 Message-ID: <7cf63e74-0ece-4af2-89e9-e08b52858195@linux.intel.com> References: <1502285302-21552-1-git-send-email-ulf.hansson@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Sender: stable-owner@vger.kernel.org To: John Stultz , Ulf Hansson Cc: Wolfram Sang , "Rafael J . Wysocki" , linux-i2c@vger.kernel.org, Andy Shevchenko , Mika Westerberg , Jisheng Zhang , Guodong Xu , Sumit Semwal , Haojian Zhuang , "linux-arm-kernel@lists.infradead.org" , Linux PM list , "# v4 . 4+" List-Id: linux-i2c@vger.kernel.org On 08/09/2017 11:55 PM, John Stultz wrote: > On Wed, Aug 9, 2017 at 6:28 AM, Ulf Hansson wrote: >> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming >> during system suspend"), may suggest to the PM core to try out the so >> called direct_complete path for system sleep. In this path, the PM core >> treats a runtime suspended device as it's already in a proper low power >> state for system sleep, which makes it skip calling the system sleep >> callbacks for the device, except for the ->prepare() and the ->complete() >> callbacks. >> >> However, the PM core may unset the direct_complete flag for a parent >> device, in case its child device are being system suspended before. In this >> scenario, the PM core invokes the system sleep callbacks, no matter if the >> device is runtime suspended or not. >> >> Particularly in cases of an existing i2c slave device, the above path is >> triggered, which breaks the assumption that the i2c device is always >> runtime resumed whenever the dw_i2c_plat_suspend() is being called. >> >> More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and >> clk_core_unprepare(), for an already disabled/unprepared clock, leading to >> a splat in the log about clocks calls being wrongly balanced and breaking >> system sleep. >> >> To still allow the direct_complete path in cases when it's possible, but >> also to keep the fix simple, let's runtime resume the i2c device in the >> ->suspend() callback, before continuing to put the device into low power >> state. >> >> Note, in cases when the i2c device is attached to the ACPI PM domain, this >> problem doesn't occur, because ACPI's ->suspend() callback, assigned to >> acpi_subsys_suspend(), already calls pm_runtime_resume() for the device. >> >> It should also be noted that this change does not fix commit 8503ff166504 >> ("i2c: designware: Avoid unnecessary resuming during system suspend"). >> Because for the non-ACPI case, the system sleep support was already broken >> prior that point. >> >> Cc: # v4.4+ >> Signed-off-by: Ulf Hansson >> --- >> >> I decided to post this as a separate change, instead of as earlier folding it >> in the series that extends the ACPI PM domain to cope with the runtime PM >> centric path for system sleep. >> >> This change applies on a fresh v4.4+. If someone wants it's applied for earlier >> version, please send a backport yourself. >> >> It's based on 4.13 rc4 and I assume it should go as a fix via the i2c tree. >> >> Changes in v2: >> - Updated changelog. >> - Runtime resume the device in ->suspend() instead of in ->prepare(). > > > This avoids the suspend/resume warning I've seen w/o the patch on HiKey. > > Tested-by: John Stultz > Cool, no issues from acpi_lpss.c nor mfd/intel-lpss.c platforms. Tested-by: Jarkko Nikula Acked-by: Jarkko Nikula