[{"id":1762556,"web_url":"http://patchwork.ozlabs.org/comment/1762556/","msgid":"<3349581.Lv4bLKTrbL@aspire.rjw.lan>","list_archive_url":null,"date":"2017-09-04T10:07:21","subject":"Re: [RFT][PATCH 2/2] PM / i2c: designware: Clean up system sleep\n\thandling without ACPI","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Monday, September 4, 2017 1:14:44 AM CEST Rafael J. Wysocki wrote:\n> From: Rafael J. Wysocki \n> \n> After commit a23318feeff6 (i2c: designware: Fix system suspend)\n> the i2c-designware-platdrv driver always resumes the device in its\n> system sleep ->suspend callback which isn't particularly nice,\n> even though it is technically correct.\n> \n> A better approach would be to make the driver track the PM state of\n> the device so that it doesn't need to resume it in ->suspend and\n> to drop its ->prepare and ->complete callbacks which would only be\n> marginally useful then, so implement it.\n> \n> First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 and\n> rename dw_i2c_plat_runtime_suspend() back to dw_i2c_plat_suspend().\n> \n> Second, point the driver's ->late_suspend and ->early_resume\n> callbacks, rather than its ->suspend and ->resume callbacks,\n> to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,\n> so that they are not executed in parallel with each other, for\n> example if runtime resume of the device takes place during system\n> suspend. Also, since the driver enables runtime PM unconditionally\n> in dw_i2c_plat_probe(), this change allows the\n> pm_runtime_status_suspended() check to be used in the PM callbacks\n> to determine whether or not the device needs to be either suspended\n> or resumed (moving the callbacks into the late/early stages of\n> system suspend/resume, respectively, guarantees the stability\n> of the runtime PM status at the time when they are invoked).\n> \n> Next, add a \"skip_resume\" flag to struct dw_i2c_dev and make\n> dw_i2c_plat_suspend() and dw_i2c_plat_resume() use it to avoid\n> resuming a previously runtime-suspended device during system resume.\n> \n> Finally, drop the driver's ->prepare and ->complete PM callbacks,\n> because returning \"true\" from ->prepare for runtime-suspended\n> devices is marginally useful (the PM core may still ignore that\n> return value and invoke the driver's ->suspend callback anyway)\n> and ->complete is only needed because of what ->prepare does.\n> \n> Overrides: a23318feeff6 (i2c: designware: Fix system suspend)\n> Signed-off-by: Rafael J. Wysocki \n> ---\n> drivers/i2c/busses/i2c-designware-core.h | 1 \n> drivers/i2c/busses/i2c-designware-platdrv.c | 47 +++++++++-------------------\n> 2 files changed, 17 insertions(+), 31 deletions(-)\n> \n> Index: linux-pm/drivers/i2c/busses/i2c-designware-core.h\n> ===================================================================\n> --- linux-pm.orig/drivers/i2c/busses/i2c-designware-core.h\n> +++ linux-pm/drivers/i2c/busses/i2c-designware-core.h\n> @@ -280,6 +280,7 @@ struct dw_i2c_dev {\n> \tint\t\t\t(*acquire_lock)(struct dw_i2c_dev *dev);\n> \tvoid\t\t\t(*release_lock)(struct dw_i2c_dev *dev);\n> \tbool\t\t\tpm_disabled;\n> +\tbool\t\t\tskip_resume;\n> \tvoid\t\t\t(*disable)(struct dw_i2c_dev *dev);\n> \tvoid\t\t\t(*disable_int)(struct dw_i2c_dev *dev);\n> \tint\t\t\t(*init)(struct dw_i2c_dev *dev);\n> Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c\n> ===================================================================\n> --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c\n> +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c\n> @@ -434,28 +434,17 @@ static const struct of_device_id dw_i2c_\n> MODULE_DEVICE_TABLE(of, dw_i2c_of_match);\n> #endif\n> \n> -#ifdef CONFIG_PM_SLEEP\n> -static int dw_i2c_plat_prepare(struct device *dev)\n> -{\n> -\treturn pm_runtime_suspended(dev);\n> -}\n> -\n> -static void dw_i2c_plat_complete(struct device *dev)\n> -{\n> -\tif (dev->power.direct_complete)\n> -\t\tpm_request_resume(dev);\n> -}\n> -#else\n> -#define dw_i2c_plat_prepare\tNULL\n> -#define dw_i2c_plat_complete\tNULL\n> -#endif\n> -\n> #ifdef CONFIG_PM\n> -static int dw_i2c_plat_runtime_suspend(struct device *dev)\n> +static int dw_i2c_plat_suspend(struct device *dev)\n> {\n> \tstruct platform_device *pdev = to_platform_device(dev);\n> \tstruct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);\n> \n> +\tif (pm_runtime_status_suspended(dev)) {\n> +\t\ti_dev->skip_resume = true;\n> +\t\treturn 0;\n> +\t}\n> +\n> \ti_dev->disable(i_dev);\n> \ti2c_dw_plat_prepare_clk(i_dev, false);\n> \n> @@ -467,27 +456,23 @@ static int dw_i2c_plat_resume(struct dev\n> \tstruct platform_device *pdev = to_platform_device(dev);\n> \tstruct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);\n> \n> +\tif (!pm_runtime_status_suspended(dev))\n\nThis check is incorrect, I've just sent a v2 of the patch.\n\nSorry about this.\n\n> +\t\treturn 0;\n> +\n> +\tif (i_dev->skip_resume) {\n> +\t\ti_dev->skip_resume = false;\n> +\t\treturn 0;\n> +\t}\n> +\n> \ti2c_dw_plat_prepare_clk(i_dev, true);\n> \ti_dev->init(i_dev);\n> \n> \treturn 0;\n> }\n> \n> -#ifdef CONFIG_PM_SLEEP\n> -static int dw_i2c_plat_suspend(struct device *dev)\n> -{\n> -\tpm_runtime_resume(dev);\n> -\treturn dw_i2c_plat_runtime_suspend(dev);\n> -}\n> -#endif\n> -\n> static const struct dev_pm_ops dw_i2c_dev_pm_ops = {\n> -\t.prepare = dw_i2c_plat_prepare,\n> -\t.complete = dw_i2c_plat_complete,\n> -\tSET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)\n> -\tSET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,\n> -\t\t\t dw_i2c_plat_resume,\n> -\t\t\t NULL)\n> +\tSET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)\n> +\tSET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)\n> };\n> \n> #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm5ND3hGNz9s4s\n\tfor ;\n\tMon, 4 Sep 2017 20:16:12 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753411AbdIDKQK (ORCPT );\n\tMon, 4 Sep 2017 06:16:10 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:58977 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1753350AbdIDKQJ (ORCPT\n\t); Mon, 4 Sep 2017 06:16:09 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 89059576a70e171f; Mon, 4 Sep 2017 12:16:07 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"linux-pm@vger.kernel.org, linux-i2c@vger.kernel.org","Cc":"Wolfram Sang , linux-acpi@vger.kernel.org,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach , Ulf Hansson ","Subject":"Re: [RFT][PATCH 2/2] PM / i2c: designware: Clean up system sleep\n\thandling without ACPI","Date":"Mon, 04 Sep 2017 12:07:21 +0200","Message-ID":"<3349581.Lv4bLKTrbL@aspire.rjw.lan>","In-Reply-To":"<6044272.h4jTMoqRfC@aspire.rjw.lan>","References":"<3023226.l5IfJK6GIc@aspire.rjw.lan>\n\t<6044272.h4jTMoqRfC@aspire.rjw.lan>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1763458,"web_url":"http://patchwork.ozlabs.org/comment/1763458/","msgid":"<1504626417.25945.244.camel@linux.intel.com>","list_archive_url":null,"date":"2017-09-05T15:46:57","subject":"Re: [RFT][PATCH 2/2] PM / i2c: designware: Clean up system sleep\n\thandling without ACPI","submitter":{"id":8583,"url":"http://patchwork.ozlabs.org/api/people/8583/","name":"Andy Shevchenko","email":"andriy.shevchenko@linux.intel.com"},"content":"On Mon, 2017-09-04 at 01:14 +0200, Rafael J. Wysocki wrote:\n> From: Rafael J. Wysocki \n> \n> After commit a23318feeff6 (i2c: designware: Fix system suspend)\n> the i2c-designware-platdrv driver always resumes the device in its\n> system sleep ->suspend callback which isn't particularly nice,\n> even though it is technically correct.\n> \n> A better approach would be to make the driver track the PM state of\n> the device so that it doesn't need to resume it in ->suspend and\n> to drop its ->prepare and ->complete callbacks which would only be\n> marginally useful then, so implement it.\n> \n> First, drop dw_i2c_plat_suspend() added by commit a23318feeff6 and\n> rename dw_i2c_plat_runtime_suspend() back to dw_i2c_plat_suspend().\n> \n> Second, point the driver's ->late_suspend and ->early_resume\n> callbacks, rather than its ->suspend and ->resume callbacks,\n> to dw_i2c_plat_suspend() and dw_i2c_plat_resume(), respectively,\n> so that they are not executed in parallel with each other, for\n> example if runtime resume of the device takes place during system\n> suspend.  Also, since the driver enables runtime PM unconditionally\n> in dw_i2c_plat_probe(), this change allows the\n> pm_runtime_status_suspended() check to be used in the PM callbacks\n> to determine whether or not the device needs to be either suspended\n> or resumed (moving the callbacks into the late/early stages of\n> system suspend/resume, respectively, guarantees the stability\n> of the runtime PM status at the time when they are invoked).\n> \n> Next, add a \"skip_resume\" flag to struct dw_i2c_dev and make\n> dw_i2c_plat_suspend() and dw_i2c_plat_resume() use it to avoid\n> resuming a previously runtime-suspended device during system resume.\n> \n> Finally, drop the driver's ->prepare and ->complete PM callbacks,\n> because returning \"true\" from ->prepare for runtime-suspended\n> devices is marginally useful (the PM core may still ignore that\n> return value and invoke the driver's ->suspend callback anyway)\n> and ->complete is only needed because of what ->prepare does.\n\n> +\tSET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend,\n> dw_i2c_plat_resume)\n\nLuckily I2C doesn't use DMA.\n\nFor cases when we have a separate DMA controller devices (CherryTrail /\nBayTrail) we have to be sure that I2C device is going to be suspended\nfirst and resumed last to be operational. DMA itself uses LATE PM ops\nIIRC (drivers/dma/dw/platform.c) and it becomes possible ordering issue\nif both drivers are on the same level of PM ops.\n\nP.S. For now, as I noted above, it would be not a problem, I think I\njust need to mention this potential issue in the future with some\ndrivers (like SPI or UART, which are using DMA) on SoCs like CherryTrail\nor if I2C going to support DMA.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmrgZ46X4z9sP3\n\tfor ;\n\tWed, 6 Sep 2017 01:47:06 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752048AbdIEPrE (ORCPT );\n\tTue, 5 Sep 2017 11:47:04 -0400","from mga11.intel.com ([192.55.52.93]:60932 \"EHLO mga11.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751763AbdIEPrC (ORCPT );\n\tTue, 5 Sep 2017 11:47:02 -0400","from fmsmga004.fm.intel.com ([10.253.24.48])\n\tby fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t05 Sep 2017 08:47:01 -0700","from smile.fi.intel.com (HELO smile) ([10.237.72.86])\n\tby fmsmga004.fm.intel.com with ESMTP; 05 Sep 2017 08:46:58 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,480,1498546800\"; d=\"scan'208\";a=\"308170760\"","Message-ID":"<1504626417.25945.244.camel@linux.intel.com>","Subject":"Re: [RFT][PATCH 2/2] PM / i2c: designware: Clean up system sleep\n\thandling without ACPI","From":"Andy Shevchenko ","To":"\"Rafael J. Wysocki\" , linux-pm@vger.kernel.org,\n\tlinux-i2c@vger.kernel.org","Cc":"Wolfram Sang , linux-acpi@vger.kernel.org,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach , Ulf Hansson ","Date":"Tue, 05 Sep 2017 18:46:57 +0300","In-Reply-To":"<6044272.h4jTMoqRfC@aspire.rjw.lan>","References":"<3023226.l5IfJK6GIc@aspire.rjw.lan>\n\t<6044272.h4jTMoqRfC@aspire.rjw.lan>","Organization":"Intel Finland Oy","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.22.6-1 ","Mime-Version":"1.0","Content-Transfer-Encoding":"8bit","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}}]