From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: RE: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains Date: Fri, 08 Mar 2019 01:01:21 +0900 Message-ID: <7htvgeekry.fsf@baylibre.com> References: <1551878926-8455-1-git-send-email-aisheng.dong@nxp.com> <1551878926-8455-2-git-send-email-aisheng.dong@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Aisheng Dong , Ulf Hansson Cc: "dongas86@gmail.com" , "khilman@kernel.org" , "linux-pm@vger.kernel.org" , "rjw@rjwysocki.net" , dl-linux-imx , "linux-arm-kernel@lists.infradead.org" List-Id: linux-pm@vger.kernel.org Aisheng Dong writes: >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Wednesday, March 6, 2019 11:18 PM> >> On Wed, 6 Mar 2019 at 14:35, Aisheng Dong >> wrote: >> > >> > Currently the generic power domain will power off the domain if all >> > devices in it have been stopped during system suspend. >> > >> > It is done by checking if the domain is active in >> > genpd_sync_power_off, then disable it. However, for power domains >> > supporting multiple low power states, it may have already entered an >> > intermediate low power state by runtime PM before system suspend and >> > the status is already GPD_STATE_POWER_OFF which results in then the >> > power domain stay at an intermediate low power state during system >> suspend. >> > Then genpd_sync_power_off will keep it at the low power state instead >> > of completely gate off it. >> >> I agree that this could be a concern. However, before we look for a solution, do >> you have practical use case where you observes this? >> > > Yes, this solution[1] is used by NXP internally for former releases that we have two > Level states for power Domains[2]. > We use state 0 (Low Power Mode with state retention) for device runtime pm support > and state 1 (power off with no state retention) for system suspend resume case. > > Without [1], we will meet the issue that all domains runtime suspended will stay at > low power mode instead of power off during system suspend which consumes more power. > > 1. https://source.codeaurora.org/external/imx/linux-imx/log/drivers/base/power?h=imx_4.14.78_1.0.0_ga > 2. https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/soc/imx/pm-domains.c?h=imx_4.14.78_1.0.0_ga#n325 > >> > >> > Let's give the power domain a chance to switch to the deepest state in >> > case it's already off but in an intermediate low power state. >> > >> > Signed-off-by: Dong Aisheng >> > --- >> > drivers/base/power/domain.c | 18 +++++++++++++++++- >> > include/linux/pm_domain.h | 1 + >> > 2 files changed, 18 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> > index 61cd500..847a69e 100644 >> > --- a/drivers/base/power/domain.c >> > +++ b/drivers/base/power/domain.c >> > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct >> > generic_pm_domain *genpd, bool use_lock, { >> > struct gpd_link *link; >> > >> > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) >> > + /* >> > + * Give the power domain a chance to switch to the deepest state >> in >> > + * case it's already off but in an intermediate low power state. >> > + */ >> > + genpd->state_idx_saved = genpd->state_idx; >> > + >> > + if (genpd_is_always_on(genpd)) >> > + return; >> > + >> > + if (!genpd_status_on(genpd) && >> > + genpd->state_idx == (genpd->state_count - 1)) >> > return; >> >> This means that the ->power_off() callback may be called twice in a raw, for >> the genpd in question and in cases of multiple idle states. >> It could be a problem. >> >> Well, currently it may not be a real issue, as I doubt there is rather few genpd >> backend drivers supporting multiple idle states, but still. >> > > So far, I've no idea what the problem could be as there's no multi states power domain > drivers in tree. A problem could arise if the drivers need to do something before they can switch from retention to OFF (e.g. save context). AFAICT without seeing the affected drivers in your solution, I can't tell if the drivers have an opportunity to save context to prepare for off-mode. > For IMX, there's no problem as the driver could handle it well. In that case, I guess you're assuming that system suspsend will always hit off-mode, so all your drivers always save context in their suspend callbacks? > The core code just provides an interface for driver to handle such requirement. I think we're up against a terminology problem/confusion since we added multiple states around what does "off" really mean. For example, should "off" mean in the deepest, possible state, or does it mean idle in the last selected state? Before we had multiple states, it always meant the former, and the code was built around the idea of really, truly off (zero volts, no retention.) With multiple states, it's now a bit ambigious. In this series, you're using it to mean both, which is part of the confusion. Stated differently, while we technically have support for mulitple states, in the core we still just have 2: enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ GPD_STATE_POWER_OFF, /* PM domain is off */ }; So bascially we have to make a decsion whether retention is _ACTIVE or _OFF. This series is assuming it should be _OFF, but when we were adding multiple states, I had always assumed retention states would be considered _ACTIVE. Yes, I know tha neither "active" nor "off" accurately defines retention, but since there is still voltage applied, it may not be active, but it's definitely not off. When we were adding multiple states, I had always imagined domains with multiple states would handle this kind of thing with a governor. Retention states whould leave the domain in GPD_STATE_ACTIVE, and the governor would then determine when to switch to the real off state. Note that this gets you more flexiblity than just switching to off in suspend. You could also use the governor to let the domain switch to off based on other criteria such as long idle time, or QoS criteria. It seems you may be the first upstream solution to start really exercising the multiple states, so we may still have some kinks to work out, but thanks for pushing your changes upstream so we can work all this out together. Thanks, Kevin From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.9 required=3.0 tests=DATE_IN_PAST_03_06, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D871FC43381 for ; Thu, 7 Mar 2019 20:13:17 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9CF3C20840 for ; Thu, 7 Mar 2019 20:13:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="dzx6Bkdb"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=baylibre-com.20150623.gappssmtp.com header.i=@baylibre-com.20150623.gappssmtp.com header.b="AeKObDjK" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9CF3C20840 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:Date:References :In-Reply-To:Subject:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=4eQvpTb1ipTu8MJpc3DhR94fDdB6ktUtk3OTE77yciQ=; b=dzx6BkdbRwn2F3 RJq0tYms7B77S6WRdvTny958BK04g7Brpe3A0D4/WlvrnGJH4Jxsgan2hCOjkjmMqR/vtZVDoOR6U 62Ywle3Jv6LtVoSTHhOfMLqw/APMgSu7IhT8eyxIvhAOzqQJzcX5ZbbixKMJwo4EixJ/S55gLrWMM IyqYSuVnLyAPJlPH+4t59DQuJgsC/cSCFdIIdeLbThTYQ9hM1+k4dUGG6JELuGcPrhk8KVUAm+YJI rUWBKxizIIayeiJLReEbe7rbltLTGht8FqoBHH2HncF9UPuCOTwWrN27YOsfHHq+rmsMM2U8fTwnx qaX5p14u1sp2G4drqu1g==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h1zOA-0000Tc-6a; Thu, 07 Mar 2019 20:13:10 +0000 Received: from mail-pf1-x435.google.com ([2607:f8b0:4864:20::435]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h1zO6-0000Sc-DV for linux-arm-kernel@lists.infradead.org; Thu, 07 Mar 2019 20:13:08 +0000 Received: by mail-pf1-x435.google.com with SMTP id g6so12283818pfh.13 for ; Thu, 07 Mar 2019 12:13:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:in-reply-to:references:date:message-id :mime-version; bh=yB7aWwjrdk6dmw3Y/ys4xdqvAxqvdhaPAq+HmBeQJAw=; b=AeKObDjKHtwERpd1FPY7W/EOZkjvqYQhsGtb9Q+4vFZh2GTNS1zQvZYqhCUS3C2/Ga EbeX9Edo7YRN9zqp2H4Ynj7Ng2V5CURbWX/XniW4uRdwNMWhWd+BoAnm9Qv6v8eBfjmP 6lJ8TuLJUP5nS9r97XDKWdoCL+Ykw1LUTLP1O8I78xJnJmsKLBcJoaAMXQzpUmNHXteC QlSgfsk7R/t2Uh3QrpDuaIEXXGG6IlQ/FICpqegxWoY0J32CCZNud+796LQno0RUhKqY Y2PFe3kBrLlSnQsv/zgBJw1g49e9J5SFxnd1w4VywR9fKldELOsCwkYYfBjLzsl3IgfL waHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=yB7aWwjrdk6dmw3Y/ys4xdqvAxqvdhaPAq+HmBeQJAw=; b=neNZhYE6j49tQAQq/qYUPUbn5gnMNOY5eWNulWI/lbvWC2jyrfes5ki1Jxoa+JLQem wbMO3Rf0XwabsvfLYEDwGodfCAuYjGfzsIO5Gyehbp8WXjOZNH2kAZz3xQV0TsMIDfrs iD6v8gGksV78kmmYo8F33XHN8nxd1esIPi/SnQ3EGbEJjEiCM2W/puxgGbggwptBOZlB j+LmxZAqF4AaFhTsBEnXq6YIX5eCLbd7Edya7/IfJ6tfxweAzIIiJEou8d0vnF7a+keX lwWDPtNKMH/0R7ZA3v7AgsnUe7Gp0m6HQmCMiHt0ZdLzT2rZpfNV3P3m824w4I3xq7KF jhEw== X-Gm-Message-State: APjAAAUAFmLu0IuuDDjqVzIienZ38svB4GRdyrjS5ubo9XvESzMZ3zCU 8UaDXvdEHaknw/Ig+vVaGlDjQQ== X-Google-Smtp-Source: APXvYqxyQ4VTt/RTc7H7n4iYWdVxUZAfz197sQhVmNMTRQK6Hp7JR8DnLmTG2gTkjnFiOVZwCBYRFQ== X-Received: by 2002:a62:4414:: with SMTP id r20mr14595861pfa.37.1551989585109; Thu, 07 Mar 2019 12:13:05 -0800 (PST) Received: from localhost ([2601:602:9200:a1a5:98f2:d444:5b9e:15fd]) by smtp.googlemail.com with ESMTPSA id o7sm19949668pfi.105.2019.03.07.12.13.03 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 07 Mar 2019 12:13:04 -0800 (PST) From: Kevin Hilman To: Aisheng Dong , Ulf Hansson Subject: RE: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains In-Reply-To: References: <1551878926-8455-1-git-send-email-aisheng.dong@nxp.com> <1551878926-8455-2-git-send-email-aisheng.dong@nxp.com> Date: Fri, 08 Mar 2019 01:01:21 +0900 Message-ID: <7htvgeekry.fsf@baylibre.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190307_121306_455959_E12044B9 X-CRM114-Status: GOOD ( 31.45 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "dongas86@gmail.com" , "khilman@kernel.org" , "linux-pm@vger.kernel.org" , "rjw@rjwysocki.net" , dl-linux-imx , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Aisheng Dong writes: >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Wednesday, March 6, 2019 11:18 PM> >> On Wed, 6 Mar 2019 at 14:35, Aisheng Dong >> wrote: >> > >> > Currently the generic power domain will power off the domain if all >> > devices in it have been stopped during system suspend. >> > >> > It is done by checking if the domain is active in >> > genpd_sync_power_off, then disable it. However, for power domains >> > supporting multiple low power states, it may have already entered an >> > intermediate low power state by runtime PM before system suspend and >> > the status is already GPD_STATE_POWER_OFF which results in then the >> > power domain stay at an intermediate low power state during system >> suspend. >> > Then genpd_sync_power_off will keep it at the low power state instead >> > of completely gate off it. >> >> I agree that this could be a concern. However, before we look for a solution, do >> you have practical use case where you observes this? >> > > Yes, this solution[1] is used by NXP internally for former releases that we have two > Level states for power Domains[2]. > We use state 0 (Low Power Mode with state retention) for device runtime pm support > and state 1 (power off with no state retention) for system suspend resume case. > > Without [1], we will meet the issue that all domains runtime suspended will stay at > low power mode instead of power off during system suspend which consumes more power. > > 1. https://source.codeaurora.org/external/imx/linux-imx/log/drivers/base/power?h=imx_4.14.78_1.0.0_ga > 2. https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/soc/imx/pm-domains.c?h=imx_4.14.78_1.0.0_ga#n325 > >> > >> > Let's give the power domain a chance to switch to the deepest state in >> > case it's already off but in an intermediate low power state. >> > >> > Signed-off-by: Dong Aisheng >> > --- >> > drivers/base/power/domain.c | 18 +++++++++++++++++- >> > include/linux/pm_domain.h | 1 + >> > 2 files changed, 18 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> > index 61cd500..847a69e 100644 >> > --- a/drivers/base/power/domain.c >> > +++ b/drivers/base/power/domain.c >> > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct >> > generic_pm_domain *genpd, bool use_lock, { >> > struct gpd_link *link; >> > >> > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) >> > + /* >> > + * Give the power domain a chance to switch to the deepest state >> in >> > + * case it's already off but in an intermediate low power state. >> > + */ >> > + genpd->state_idx_saved = genpd->state_idx; >> > + >> > + if (genpd_is_always_on(genpd)) >> > + return; >> > + >> > + if (!genpd_status_on(genpd) && >> > + genpd->state_idx == (genpd->state_count - 1)) >> > return; >> >> This means that the ->power_off() callback may be called twice in a raw, for >> the genpd in question and in cases of multiple idle states. >> It could be a problem. >> >> Well, currently it may not be a real issue, as I doubt there is rather few genpd >> backend drivers supporting multiple idle states, but still. >> > > So far, I've no idea what the problem could be as there's no multi states power domain > drivers in tree. A problem could arise if the drivers need to do something before they can switch from retention to OFF (e.g. save context). AFAICT without seeing the affected drivers in your solution, I can't tell if the drivers have an opportunity to save context to prepare for off-mode. > For IMX, there's no problem as the driver could handle it well. In that case, I guess you're assuming that system suspsend will always hit off-mode, so all your drivers always save context in their suspend callbacks? > The core code just provides an interface for driver to handle such requirement. I think we're up against a terminology problem/confusion since we added multiple states around what does "off" really mean. For example, should "off" mean in the deepest, possible state, or does it mean idle in the last selected state? Before we had multiple states, it always meant the former, and the code was built around the idea of really, truly off (zero volts, no retention.) With multiple states, it's now a bit ambigious. In this series, you're using it to mean both, which is part of the confusion. Stated differently, while we technically have support for mulitple states, in the core we still just have 2: enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ GPD_STATE_POWER_OFF, /* PM domain is off */ }; So bascially we have to make a decsion whether retention is _ACTIVE or _OFF. This series is assuming it should be _OFF, but when we were adding multiple states, I had always assumed retention states would be considered _ACTIVE. Yes, I know tha neither "active" nor "off" accurately defines retention, but since there is still voltage applied, it may not be active, but it's definitely not off. When we were adding multiple states, I had always imagined domains with multiple states would handle this kind of thing with a governor. Retention states whould leave the domain in GPD_STATE_ACTIVE, and the governor would then determine when to switch to the real off state. Note that this gets you more flexiblity than just switching to off in suspend. You could also use the governor to let the domain switch to off based on other criteria such as long idle time, or QoS criteria. It seems you may be the first upstream solution to start really exercising the multiple states, so we may still have some kinks to work out, but thanks for pushing your changes upstream so we can work all this out together. Thanks, Kevin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel